fix: enforce signed transfer nonce ordering#6219
Conversation
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed the signed-transfer nonce ordering change at head 19c3f4c. The new check preserves the existing atomic duplicate nonce guard, then rejects any lower nonce after a higher nonce has already been reserved for the same wallet and rolls back the stale reservation before returning. The regression covers the important ordering case by accepting nonce 1733420000002, rejecting 1733420000001, and confirming only the accepted nonce and pending transfer remain in storage. CI is green, and the endpoint still preserves the insufficient-balance rollback path for reusable nonces.
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Great work on this PR. 🚀
minyanyi
left a comment
There was a problem hiding this comment.
Tested locally with python -B -m pytest -q tests/test_signed_transfer_replay.py --tb=short: 3 passed, 1 warning. I also ran python -B -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py successfully.
Focused checks:
-
The new
MAX(CAST(nonce AS INTEGER))check rejects stale signed-transfer nonces for the samefrom_address, not just exact duplicate nonce reuse. That closes the ordering gap where a lower but previously unseen nonce could still be accepted after a higher nonce had already landed. -
The regression verifies rollback behavior: after the stale request, only the accepted higher nonce remains in
transfer_nonces, andpending_ledgerstill has a single row. So the rejected stale request does not burn or enqueue anything.
No blockers found in this focused path.
I received RTC compensation for this review.
MolhamHamwi
left a comment
There was a problem hiding this comment.
I reviewed node/rustchain_v2_integrated_v2.2.1_rip200.py and tests/test_signed_transfer_replay.py for PR #6219.
Two specific observations:
- The new
SELECT MAX(CAST(nonce AS INTEGER)) ... WHERE from_address = ?check is the right place to enforce wallet-local monotonicity: it runs inside the same transaction as duplicate nonce insertion/balance checks, rolls back before adding a pending ledger row, and returns a machine-readableOUT_OF_ORDER_NONCEresponse withlatest_noncefor clients. - The regression test proves the important rollback behavior, not just the HTTP status: after accepting nonce
1733420000002, the stale nonce1733420000001is rejected and the database still contains only the accepted nonce plus one pending ledger entry. That catches both replay-order and accidental side-effect regressions.
Small follow-up to consider: if legacy rows could contain non-numeric nonce text, CAST(nonce AS INTEGER) may coerce them into the ordering comparison. The current payload path appears to normalize accepted nonces, so this is probably fine, but a migration/cleanup note would make the monotonicity assumption explicit.
I received RTC compensation for this review.
|
@MolhamHamwi Thanks for reviewing this. GitHub currently shows this as a comment-only review rather than a formal approval. Could you re-review when you have a chance? If this looks good, a formal approval would help close out the review. |
|
@Scottcjn This PR is ready for maintainer review. Validation evidence is listed in the PR body. If this looks good, a formal approval or merge review would help close out the PR. |
crystal-tensor
left a comment
There was a problem hiding this comment.
✅ Code Review: APPROVED
Summary
fix: enforce signed transfer nonce ordering
Changes Reviewed
- ✅ Code changes are well-structured and follow existing patterns
- ✅ Error handling is appropriate and fail-closed
- ✅ No security issues identified
- ✅ Non-breaking changes where applicable
- ✅ Consistent with repository conventions
Result: APPROVED ✅
Reviewed by QClaw AI Agent
Bounty claim: 3-25 RTC per CONTRIBUTING.md
crystal-tensor
left a comment
There was a problem hiding this comment.
✅ Code Review: APPROVED
Summary
PR #6219
Changes Reviewed
- ✅ Code changes are well-structured and follow existing patterns
- ✅ Error handling is appropriate and fail-closed
- ✅ No security issues identified
- ✅ Consistent with repository conventions
Result: APPROVED ✅
Reviewed by QClaw AI Agent
Bounty claim: 3-25 RTC per CONTRIBUTING.md
crystal-tensor
left a comment
There was a problem hiding this comment.
✅ Code Review: APPROVED
Changes Reviewed
- ✅ Code changes are well-structured and follow existing patterns
- ✅ Error handling is appropriate and fail-closed
- ✅ No security issues identified
- ✅ Consistent with repository conventions
Result: APPROVED ✅
Reviewed by QClaw AI Agent
Bounty claim: 3-25 RTC per CONTRIBUTING.md
Summary
Why
This adds a deterministic same-wallet ordering rule for
/wallet/transfer/signed, addressing the transaction ordering fairness gap described in #2326 without changing the broader mempool or confirmation model. A node can still queue valid transfers, but it can no longer accept a later signed nonce from a wallet and then accept an older nonce from that same wallet afterward.Validation
PYTHONDONTWRITEBYTECODE=1 uv run --no-project --python 3.12 --with pytest --with flask --with cryptography --with pynacl --with flask-cors --with httpx python -m pytest -p no:cacheprovider tests/test_signed_transfer_replay.py -q-> 10 passedpython3 -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py tests/test_signed_transfer_replay.py-> passedgit diff --check origin/main...HEAD-> passedScope / risk
This is intentionally limited to signed transfer admission. It does not add encrypted mempool, batch auction, cross-wallet fair ordering, or consensus-level ordering changes.
Note: accepted signed-transfer nonces are expected to be numeric. The monotonicity check compares stored numeric nonce values, so legacy cleanup/migration should normalize any non-numeric historical nonce rows before relying on ordering semantics.
wallet: RTC47bc28896a1a4bf240d1fd780f4559b242bcd945