fix: harden RTC auto-pay idempotency markers#5562
Conversation
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150) A maintainer will review your PR soon. Thanks for contributing! |
github-actions
Bot
added
BCOS-L1
iamdinhthuan
force-pushed
the
fix-auto-pay-idempotency-markers
branch
from
bae1544 to
a4cdba0
Compare
kekehanshujun
left a comment
kekehanshujun
left a comment
There was a problem hiding this comment.
Reviewing this as part of the RustChain code review bounty.
The idempotency key and trusted-marker checks are useful, but the new RTC-AutoPay-Started marker creates a payment-liveness bug. The workflow posts the started marker before calling /wallet/transfer; if the transfer then fails with ConnectionError, timeout, a 5xx, or any exception before a confirmed/failed marker is posted, the started marker remains on the PR. On the next run, find_existing_payment_marker() treats that trusted started marker as final enough to skip, so the contributor may never be paid.
The tests cover a failed confirmation-comment post after a successful transfer, but not a failed transfer after the started marker is posted. Please either stop treating RTC-AutoPay-Started as a skip condition unless it has a bounded freshness window and a matching pending transfer, or post the started marker only after the transfer endpoint has accepted an idempotent request. Add a regression where transfer_rtc raises after the started comment and the next run still retries the transfer.
|
Addressed the liveness issue in commit 6452cc0. Added/updated regressions:
Validation run on the PR branch: python3 -m pytest -q scripts/test_auto_pay_idempotency.py .github/actions/rtc-auto-bounty/test_award_rtc.py
python3 -m py_compile scripts/auto-pay.py scripts/test_auto_pay_idempotency.py .github/actions/rtc-auto-bounty/award_rtc.py
git diff --checkResult: I did not bundle CI dependency changes here because missing full-suite dependencies are already covered separately by open PR #5350; this PR stays scoped to auto-pay idempotency/liveness. |
2balmprune
left a comment
2balmprune
left a comment
There was a problem hiding this comment.
Reviewed updated head 6452cc03858bea53c96e4633429efc8aaf84b8f1 after the RTC-AutoPay-Started liveness follow-up.
The earlier blocker is resolved on this head: RTC-AutoPay-Started is no longer treated as a final skip marker, so a transfer connection failure after the started comment can retry with the same idempotency_key. Final trusted RTC-AutoPay-Confirmed markers still stop reruns, and untrusted confirmation markers no longer suppress an owner payment directive. The key is stable for the repo/PR/payment comment/amount/recipient tuple, so a confirmation-comment failure can rerun without changing the transfer idempotency key.
Validation performed:
git diff --check origin/main...HEAD -- scripts/auto-pay.py scripts/test_auto_pay_idempotency.pypython -B -m py_compile scripts/auto-pay.py scripts/test_auto_pay_idempotency.pypython -B -m pytest -q scripts/test_auto_pay_idempotency.py-> 3 passedpython -B -m py_compile .github/actions/rtc-auto-bounty/award_rtc.py .github/actions/rtc-auto-bounty/test_award_rtc.pypython .github/actions/rtc-auto-bounty/test_award_rtc.py-> 40 tests passed
No remaining blocker from my review.
ZacharyZhang-NY
left a comment
ZacharyZhang-NY
left a comment
There was a problem hiding this comment.
I found a blocking duplicate-payment risk in the proposed idempotency flow. The script now sends idempotency_key to POST /wallet/transfer, but the current wallet_transfer_v2 endpoint in node/rustchain_v2_integrated_v2.2.1_rip200.py does not read or persist idempotency_key; rg only finds the field in scripts/auto-pay.py. That means a rerun after transfer success plus confirmation-comment failure will call /wallet/transfer again, and the server will create a second pending transfer.\n\nEvidence from this PR head 6452cc0:\n- scripts/auto-pay.py posts RTC-AutoPay-Started before transfer, but find_existing_payment_marker() only checks RTC-AutoPay-Confirmed, so a started marker does not stop the rerun.\n- node/rustchain_v2_integrated_v2.2.1_rip200.py:/wallet/transfer validates from_miner/to_miner/amount/reason and inserts a new pending_ledger row with a fresh tx_hash; it has no idempotency_key lookup or uniqueness guard.\n- A local probe simulating confirmation-comment failure ran main() twice and produced transfer_calls=2 with the same idempotency_key. Against the current server implementation those are two distinct transfer attempts.\n\nValidation performed:\n- python -B -m py_compile scripts/auto-pay.py scripts/test_auto_pay_idempotency.py .github/actions/rtc-auto-bounty/award_rtc.py\n- git diff --check origin/main...HEAD -- scripts/auto-pay.py scripts/test_auto_pay_idempotency.py\n- PYTHONPATH=scripts python -B -m pytest -q scripts/test_auto_pay_idempotency.py --noconftest -> 3 passed\n- python -B -m pytest -q scripts/test_auto_pay_idempotency.py .github/actions/rtc-auto-bounty/test_award_rtc.py --noconftest -> 43 passed\n- Manual non-idempotent-server probe: two transfer calls after confirmation-comment failure, same idempotency_key.\n\nPlease either implement server-side idempotency for /wallet/transfer before relying on this field, or make the workflow's trusted Started marker block reruns after the transfer boundary is crossed and require manual reconciliation.
2balmprune
left a comment
2balmprune
left a comment
There was a problem hiding this comment.
Follow-up after rechecking head 6452cc03858bea53c96e4633429efc8aaf84b8f1: changing my prior approval to request changes.
The liveness fix handles the started-marker retry path, but the duplicate-transfer guard still relies on /wallet/transfer honoring idempotency_key. I do not see that support in the current server endpoint. scripts/auto-pay.py sends idempotency_key in the payload, but node/rustchain_v2_integrated_v2.2.1_rip200.py::wallet_transfer_v2() validates the admin payload and inserts a fresh pending_ledger row with a new tx hash/pending id; it does not read or persist the idempotency key. The new regression mocks idempotent server behavior with accepted_transfers.setdefault(key, ...), so it proves the client reuses the key but not that the real transfer endpoint dedupes it.
Failure case:
- auto-pay posts
RTC-AutoPay-Started /wallet/transfersucceeds and creates a pending transfer- posting
RTC-AutoPay-Confirmedfails - rerun does not treat started as final, so it calls
/wallet/transferagain with the sameidempotency_key - current server creates another pending transfer because the key is ignored
Please either implement idempotency support in the transfer endpoint by storing/checking the payment key before creating a new pending transfer, or keep the workflow in manual-fallback/non-green state after a successful transfer but failed confirmation so maintainers cannot accidentally rerun into a second pending payout.
Validation:
rg -n "idempotency_key|wallet_transfer|/wallet/transfer|pending_id" scripts node .github/actionspython -B -m pytest -q scripts/test_auto_pay_idempotency.py-> 3 passedpython .github/actions/rtc-auto-bounty/test_award_rtc.py-> 40 tests passed
The tests pass, but the real endpoint does not enforce the idempotency property the client now depends on.
|
Addressed the auto-pay retry blocker in commit What changed:
Verification:
|
TJCurnutte
left a comment
TJCurnutte
left a comment
There was a problem hiding this comment.
Approved. I validated the auto-pay idempotency hardening at head 5d3df088bbb56da56eeff905a5f38b4d093e4b9b.
Proof run from a detached PR worktree:
git diff --check origin/main...HEAD -- node/rustchain_v2_integrated_v2.2.1_rip200.py scripts/auto-pay.py scripts/test_auto_pay_idempotency.py tests/test_wallet_transfer_admin_idempotency.py
python3 -B -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py scripts/auto-pay.py scripts/test_auto_pay_idempotency.py tests/test_wallet_transfer_admin_idempotency.py
python3 -B -m pytest -q scripts/test_auto_pay_idempotency.py tests/test_wallet_transfer_admin_idempotency.py
# 5 passed, 1 warning in 0.07sThe retry model now has the right boundary: scripts/auto-pay.py derives a stable payment_key from repo, PR, owner payment-comment id, amount, and recipient; untrusted confirmation markers are ignored; the new RTC-AutoPay-Started marker does not suppress a retry; and the transfer request sends that key as idempotency_key.
The node-side /wallet/transfer path validates the key shape, derives a deterministic tx_hash, returns the existing pending transfer on an exact retry, and rejects reused keys with changed transfer parameters as 409 idempotency_key_conflict without inserting a second ledger row. The focused tests cover comment-post failure retry, transfer connection retry, exact pending-ledger reuse, and changed-transfer conflict.
Live duplicate gates were clear before posting: PR open/non-draft, not self-authored, no existing @TJCurnutte PR review, and no current @TJCurnutte bounty-issue claim for this PR.
2balmprune
left a comment
2balmprune
left a comment
There was a problem hiding this comment.
Reviewed updated head 5d3df088bbb56da56eeff905a5f38b4d093e4b9b after the server-side idempotency-key follow-up. Approved.
This resolves my previous blocker: /wallet/transfer now validates an optional idempotency_key, derives a stable tx_hash from it, looks up the existing pending transfer under the write lock, returns the same pending_id/tx_hash on identical retries, and returns 409 idempotency_key_conflict if the same key is reused with changed transfer details. That closes the duplicate-pending-transfer path for the auto-pay retry case where transfer creation succeeds but the confirmation comment fails.
Validation performed:
git diff --check origin/main...HEAD -- node/rustchain_v2_integrated_v2.2.1_rip200.py scripts/auto-pay.py scripts/test_auto_pay_idempotency.py tests/test_wallet_transfer_admin_idempotency.pypython3 -B -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py scripts/auto-pay.py scripts/test_auto_pay_idempotency.py tests/test_wallet_transfer_admin_idempotency.py- Static recheck of
wallet_transfer_v2()confirmed identical idempotency-key retries return the existing pending row before balance/pending-debit checks, while changed transfer details conflict instead of inserting another row.
I could not run the Flask-backed pytest locally because this review environment is missing the Flask/pytest dependencies, but the patch logic addresses the blocker I raised.
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Great work on this PR. 🚀
8 participants
Summary
RTC-AutoPay-Confirmedmarkers unless they were posted by the repo owner orgithub-actions[bot]RTC-AutoPay-Startedmarker before transfer and include the same key as the RustChainidempotency_keyWhy
A random PR commenter could spoof the old confirmation marker and suppress a legitimate owner-approved payout. The old flow also transferred RTC before writing any durable workflow-owned marker, so a successful transfer followed by a failed confirmation comment could be retried as a second transfer.
Validation
Observed locally:
42 passed, py_compile passed, andgit diff --checkwas clean.Duplicate / safety checks
Before opening this PR I searched public issues and PRs for
RTC-AutoPay-Confirmed marker spoof idempotency,auto-pay duplicate transfer confirmation comment failure,idempotency_key auto-pay,RTC-AutoPay-Started payment_key, andscripts/auto-pay.py idempotency. I did not find an exact duplicate. PR #5359 is related to transient retry handling in.github/actions/rtc-auto-bounty/award_rtc.py, but it does not touchscripts/auto-pay.pyor the trusted marker/idempotency-key flow fixed here.No private keys, tokens, credentials, or account-internal data are included.