fix(transaction): avoid eager reservation of nonce and stray pending tx#8
Conversation
aa-eyup
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| #[tokio::test] | ||
| async fn reservation_rolls_back_when_dropped() { | ||
| let manager = NonceManager::new(TransactionNonce::new(7)); |
|
|
||
| { | ||
| let reservation = manager.reserve_next().await; | ||
| assert_eq!(reservation.nonce(), TransactionNonce::new(7)); |
| assert_eq!(reservation.nonce(), TransactionNonce::new(7)); | ||
| } | ||
|
|
||
| assert_eq!(manager.get_current_nonce().await, TransactionNonce::new(7)); |
|
|
||
| #[tokio::test] | ||
| async fn reservation_commit_keeps_increment() { | ||
| let manager = NonceManager::new(TransactionNonce::new(7)); |
| let manager = NonceManager::new(TransactionNonce::new(7)); | ||
|
|
||
| let reservation = manager.reserve_next().await; | ||
| assert_eq!(reservation.nonce(), TransactionNonce::new(7)); |
| ); | ||
| inmempool.push_back(competitive); | ||
|
|
||
| let mined_transaction = transaction_with_nonce(TransactionStatus::MINED, 12); |
| ) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(next_nonce, TransactionNonce::new(13)); |
| #[test] | ||
| fn idempotent_resolve_returns_matching_transaction_with_hash() { | ||
| let relayer_id = RelayerId::new(); | ||
| let mut transaction = transaction_with_nonce(TransactionStatus::PENDING, 1); |
| #[test] | ||
| fn idempotent_resolve_rejects_payload_mismatch() { | ||
| let relayer_id = RelayerId::new(); | ||
| let mut transaction = transaction_with_nonce(TransactionStatus::PENDING, 1); |
| #[test] | ||
| fn idempotent_resolve_returns_bad_request_for_prior_failed_simulation() { | ||
| let relayer_id = RelayerId::new(); | ||
| let mut transaction = transaction_with_nonce(TransactionStatus::FAILED, 1); |
…load mismatch - the external_id should be obeyed as the consumer defines the mapping of external_id to transaction content
| #[test] | ||
| fn idempotent_resolve_returns_accepted_transaction_with_payload_mismatch() { | ||
| let relayer_id = RelayerId::new(); | ||
| let mut transaction = transaction_with_nonce(TransactionStatus::PENDING, 1); |
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn reservation_commit_keeps_increment() { |
There was a problem hiding this comment.
There's a few more test case to do here to help me know how this works:
- What happens if transatciont none reserves mulitple (7, 8, 9, 10), then drops 8.
- What happens if transatciont none reserves mulitple (7, 8, 9, 10), then commits 10, then drops 8.
We should see expected behaviour for both, which is either to re-use oldest nonce and next reserve gets 8, then next to jump up to 11 after that, or to then ignore the dropped 8 entirely if we can.
There was a problem hiding this comment.
The mutex guard inside NonceReservation makes those scenarios impossible because reserve_next() holds a OwnedMutexGuard until commit/drop. A second call blocks until the first resolves. You can't have 7, 8, 9, 10 reserved at the same time; there's at most one outstanding reservation per NonceManager.
Serialization may come at cost of throughput, but we get correctness (no gaps, no out-of-order commits, rollback-on-drop is trivial because only one reservation is ever live). IMO its reasonable given nonce correctness is the load-bearing property. Additionally ours, Push's use case of rrelayer is not latency sensitive (the lock is held at the TransactionsQueue level and there is one constructed per relayer (aka signer).
3 participants
Motivation
POST/transactions/relayers/:relayer_id/sendcould reserve and advance a relayer nonce before gas estimation/simulation had fully succeeded. If simulation reverted, the API returned a server error and could leave aPENDINGtransaction row for a transaction that was never broadcast. Client retries could then consume future nonces due to a lack of idempotent handling.This PR makes sends safer by moving nonce reservation later (while holding a lock from the beginning of the send lifecycle), classifying deterministic simulation failures as client errors, and adding optional per-relayer external_id idempotency so clients can safely retry accepted or failed sends without consuming additional nonces.
Changes
external_ididempotency with a partial unique index.FAILED, notPENDING.external_ids before broadcasting.Flow Diagram