Move next nonce logic from Keystore to Broadcaster #10108

amit-momin · 2023-08-07T23:15:21Z

BCI-1552

Maintain the next nonce within a map in memory in the Broadcaster
- Load on startup using highest sequence + 1 in evm.txes for address
- If not found in evm.txes, check on-chain using PendingNonceAt on the client
- Added methods equivalent to the existing ones in KeyStore to maintain the next sequence map
Removed reference to next_nonce from KeyStore
- Replaced references to next nonce methods with the new Broadcaster ones
- Dropped next_nonce column from evm.key_states table
- Dropped next_nonce column from keys table
Refactored portion of the NonceSyncer Sync method to fit new logic
Added a GenerateNextSequence func type
- Allow for a chain to specify its unique way to generate the next usable sequence (e.g. for ETH nonce + 1)
- Passed to the Broadcaster as a parameter to maintain the next sequence map
Removed the set-next-nonce flag from the eth keys command since nonces are no longer stored in key state

github-actions · 2023-08-07T23:15:41Z

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

github-actions · 2023-09-11T16:32:29Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6149186608

github-actions · 2023-09-11T17:04:45Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6149498519

github-actions · 2023-09-11T17:05:09Z

Running downstream job at https://github.com/smartcontractkit/operator-ui/actions/runs/6149503267

common/txmgr/broadcaster.go

prashantkumar1982 · 2023-10-11T13:55:07Z

common/txmgr/sequence_syncer.go

-type SequenceSyncer[ADDR types.Hashable, TX_HASH types.Hashable, BLOCK_HASH types.Hashable] interface {
-	Sync(ctx context.Context, addr ADDR) (err error)
+type SequenceSyncer[ADDR types.Hashable, TX_HASH types.Hashable, BLOCK_HASH types.Hashable, SEQ types.Sequence] interface {
+	Sync(ctx context.Context, addr ADDR, localNonce SEQ) (SEQ, error)


nit: rename locaNonce to localSequence

prashantkumar1982 · 2023-10-11T14:00:25Z

core/chains/evm/txmgr/evm_tx_store.go

+	qq := o.q.WithOpts(pg.WithParentCtx(ctx))
+
+	sql := `SELECT MAX(nonce) FROM evm.txes WHERE from_address = $1 and evm_chain_id = $2`
+	err = qq.Get(&nonce, sql, fromAddress.String(), chainId.String())


So lets say there was no entry of evm.txes found for an address. It can happen if the address hasn't been used for some time, and the Reaper cleaned up previous txes that were finalized.
In that case, this query will return 0.
Thus, the caller of this function should handle the 0 case, by fetching nonce from onchain, right?

common/txmgr/broadcaster.go

prashantkumar1982 · 2023-10-11T14:05:22Z

docs/core/BULLETPROOF_TX_MANAGER_ARCHITECTURE.md

Could you please rename the file to TX_MANAGER_ARCHITECTURE?
This seemed to have missed a previous name change.

jmank88 · 2023-10-11T21:41:17Z

core/chains/evm/txmgr/evm_tx_store.go

@@ -957,6 +957,16 @@ func (o *evmTxStore) FindReceiptsPendingConfirmation(ctx context.Context, blockN
 	return
 }

+func (o *evmTxStore) FindHighestSequence(ctx context.Context, fromAddress common.Address, chainId *big.Int) (nonce evmtypes.Nonce, err error) {


Is this an EVM only signature? If not, would 'latest' or 'newest' be more general than 'highest'?

Good point. This is generalized for all chains so I can update it to FindLatestSequence

prashantkumar1982

This is great!

cl-sonarqube-production · 2023-10-12T04:52:54Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

88.5% Coverage
0.0% Duplication

amit-momin force-pushed the txm/BCI-1552 branch 12 times, most recently from ff138d2 to e90fa0c Compare August 11, 2023 19:00

amit-momin force-pushed the txm/BCI-1552 branch 8 times, most recently from 2d1d970 to 1c35d23 Compare August 22, 2023 22:48

amit-momin force-pushed the txm/BCI-1552 branch 3 times, most recently from a381832 to 0785d14 Compare August 28, 2023 17:34

amit-momin force-pushed the txm/BCI-1552 branch from 0785d14 to 1697ecb Compare September 11, 2023 16:31

amit-momin force-pushed the txm/BCI-1552 branch 2 times, most recently from eeb93be to 5e9ed68 Compare September 11, 2023 17:04

amit-momin dismissed jmank88’s stale review via 572dac7 October 10, 2023 22:27

amit-momin force-pushed the txm/BCI-1552 branch from f28e404 to 572dac7 Compare October 10, 2023 22:27

jmank88 previously approved these changes Oct 10, 2023

View reviewed changes

amit-momin dismissed jmank88’s stale review via 0c67dd0 October 10, 2023 23:27

amit-momin force-pushed the txm/BCI-1552 branch from 572dac7 to 0c67dd0 Compare October 10, 2023 23:27

jmank88 mentioned this pull request Oct 11, 2023

[BCF-2689] Use an in-memory keystore for tests #10897

Closed

prashantkumar1982 reviewed Oct 11, 2023

View reviewed changes

amit-momin force-pushed the txm/BCI-1552 branch 2 times, most recently from 3dd587e to 7539ec3 Compare October 11, 2023 21:20

jmank88 reviewed Oct 11, 2023

View reviewed changes

jmank88 previously approved these changes Oct 11, 2023

View reviewed changes

amit-momin added 9 commits October 11, 2023 16:53

Moved next_nonce logic from keystore to broadcaster

7909892

Changed sequence Increment method name to Next

104efef

Addressed PR feedback

1b6c86d

Fixed changelog entry

18e1132

Addressed PR feedback

c3df6f3

Updated to pass the next seq func as a parameter to the broadcaster

bb5c3b6

Removed unnecessary mutex initialization

a840f00

Addressed PR feedback

dfac797

Changed FindHighestSequence method name to FindLatestSequence

93067b0

amit-momin dismissed jmank88’s stale review via 93067b0 October 11, 2023 21:54

amit-momin force-pushed the txm/BCI-1552 branch from bec4a0a to 93067b0 Compare October 11, 2023 21:54

jmank88 approved these changes Oct 11, 2023

View reviewed changes

prashantkumar1982 approved these changes Oct 12, 2023

View reviewed changes

prashantkumar1982 added this pull request to the merge queue Oct 12, 2023

Merged via the queue into develop with commit 4983951 Oct 12, 2023
83 checks passed

prashantkumar1982 deleted the txm/BCI-1552 branch October 12, 2023 11:43

snehaagni mentioned this pull request Nov 14, 2023

release/2.7.0 -> master #11290

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move next nonce logic from Keystore to Broadcaster #10108

Move next nonce logic from Keystore to Broadcaster #10108

amit-momin commented Aug 7, 2023 •

edited by jira bot

Loading

github-actions bot commented Aug 7, 2023

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

prashantkumar1982 Oct 11, 2023

prashantkumar1982 Oct 11, 2023

prashantkumar1982 Oct 11, 2023

jmank88 Oct 11, 2023

amit-momin Oct 11, 2023 •

edited

Loading

prashantkumar1982 left a comment

cl-sonarqube-production bot commented Oct 12, 2023

Move next nonce logic from Keystore to Broadcaster #10108

Move next nonce logic from Keystore to Broadcaster #10108

Conversation

amit-momin commented Aug 7, 2023 • edited by jira bot Loading

BCI-1552

github-actions bot commented Aug 7, 2023

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

prashantkumar1982 Oct 11, 2023

Choose a reason for hiding this comment

prashantkumar1982 Oct 11, 2023

Choose a reason for hiding this comment

prashantkumar1982 Oct 11, 2023

Choose a reason for hiding this comment

jmank88 Oct 11, 2023

Choose a reason for hiding this comment

amit-momin Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

prashantkumar1982 left a comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Oct 12, 2023

amit-momin commented Aug 7, 2023 •

edited by jira bot

Loading

amit-momin Oct 11, 2023 •

edited

Loading