-
-
Notifications
You must be signed in to change notification settings - Fork 42
Security: signed transfers must use pending_ledger 2-phase commit #127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Security: signed transfers must use pending_ledger 2-phase commit #127
Conversation
|
Review status: nearly ready but still blocked on deployment compatibility. Current live deployment executes a single script file at Please patch to use a compatibility import pattern, e.g. try local import first, then package import fallback:
After that lands, I can fast-merge and queue payout for bounty #59. |
Scottcjn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review: PR #127 — Payout Preflight (Bounty #59)
Concept: Good approach to pending transfer validation. The core logic (balance check, duplicate detection, amount validation) addresses the bounty requirements.
Issues to Fix
1. Import path breaks production (CRITICAL)
from node.payout_preflight import PayoutPreflightProduction server runs from /root/rustchain/ — there is no node package. This import will crash the server on startup. The file should be imported directly:
from payout_preflight import PayoutPreflight2. No test evidence against live API
The PR doesn't include any evidence of testing against the actual RustChain API (https://50.28.86.131). The /pending/confirm endpoint has specific field names and behaviors — please verify your validation logic matches the real server responses.
3. Minor: codex/ branch prefix
This appears to be generated via GitHub Copilot Workspace. That's fine, but please verify the code works against the real endpoint before requesting merge.
Verdict
Correct concept, needs the import path fix and live testing. Fix the import and provide test output showing it works against the real API, then this can be merged.
|
Patched deployment-compat import issue.
Import pattern used: try:
from payout_preflight import validate_wallet_transfer_admin, validate_wallet_transfer_signed
except ImportError:
from node.payout_preflight import validate_wallet_transfer_admin, validate_wallet_transfer_signedCommit: 64754b2. |
Scottcjn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review: Signed Transfers 2-Phase Commit
The Finding Is Valid
You correctly identified that signed transfers (/wallet/transfer/signed) execute immediately while admin transfers go through pending_ledger with a 24h delay. This IS an inconsistency worth discussing.
However, This May Be Intentional by Design
- Admin transfers use an API key (can be compromised, shared) → 24h delay as safety net
- Signed transfers use Ed25519 cryptographic signatures (private key never leaves the client) → immediate execution is justified because the proof of authorization is stronger
Implementation Problems
-
from node.payout_preflight import ...breaks production — the server runs as a standalone script (python3 rustchain_v2_integrated_v2.2.1_rip200.py), not a Python package. This will crash on startup. -
Stale diff base — this PR diffs against an old version of the server. The current main branch has additional features that would cause merge conflicts.
-
Stacks PR #126 files — this PR includes identical copies of
payout_preflight.pyand its tests from PR #126. -
from_minercolumn stores RTC addresses — the pending_ledger was designed for miner IDs ("dual-g4-125"), but this PR inserts RTC wallet addresses ("RTCa1b2c3d4..."). Mixing ID formats in the same column creates confusion in the confirm workflow.
What Would Be Needed
If we decide signed transfers SHOULD use pending_ledger:
- Changes must be inline in the main server file (no separate modules)
- Rebase against current main branch
- Integration test against the running server
- Discussion on whether this is actually desired (Ed25519 verification is already strong auth)
|
Merged and deployed. Follow-up hotfix |
Bounty #59 (Pending Transfer Exploits) finding: confirmation bypass.
Issue #59 describes 2-phase commit pending transfers and lists POST /wallet/transfer/signed as the endpoint that should create a pending transfer. Current implementation executes immediate balance/ledger updates, which bypasses the 24h pending window and undermines the intended safety delay.
Fix:
Notes: