Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move next nonce logic from Keystore to Broadcaster #10108

Merged
merged 9 commits into from
Oct 12, 2023
Merged

Move next nonce logic from Keystore to Broadcaster #10108

merged 9 commits into from
Oct 12, 2023

Conversation

amit-momin
Copy link
Contributor

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

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
Copy link
Contributor

github-actions bot commented Aug 7, 2023

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

@amit-momin 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 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 amit-momin force-pushed the txm/BCI-1552 branch 3 times, most recently from a381832 to 0785d14 Compare August 28, 2023 17:34
@github-actions
Copy link
Contributor

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

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

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

@github-actions
Copy link
Contributor

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

jmank88
jmank88 previously approved these changes Oct 10, 2023
View reviewed changes
@jmank88 jmank88 mentioned this pull request Oct 11, 2023
Closed
prashantkumar1982
prashantkumar1982 reviewed Oct 11, 2023
View reviewed changes
common/txmgr/broadcaster.go Show resolved Hide resolved
common/txmgr/sequence_syncer.go Outdated
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename locaNonce to localSequence

core/chains/evm/txmgr/evm_tx_store.go Outdated
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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
docs/core/BULLETPROOF_TX_MANAGER_ARCHITECTURE.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@amit-momin amit-momin force-pushed the txm/BCI-1552 branch 2 times, most recently from 3dd587e to 7539ec3 Compare October 11, 2023 21:20
jmank88
jmank88 reviewed Oct 11, 2023
View reviewed changes
core/chains/evm/txmgr/evm_tx_store.go Outdated
@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@amit-momin amit-momin Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jmank88
jmank88 previously approved these changes Oct 11, 2023
View reviewed changes
prashantkumar1982
prashantkumar1982 approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@prashantkumar1982 prashantkumar1982 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

@cl-sonarqube-production
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

88.5% 88.5% Coverage
0.0% 0.0% Duplication

@prashantkumar1982 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 prashantkumar1982 deleted the txm/BCI-1552 branch October 12, 2023 11:43
@snehaagni snehaagni mentioned this pull request Nov 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prashantkumar1982 prashantkumar1982 prashantkumar1982 approved these changes

@jmank88 jmank88 jmank88 approved these changes

@jkongie jkongie Awaiting requested review from jkongie

@samsondav samsondav Awaiting requested review from samsondav

@RyanRHall RyanRHall Awaiting requested review from RyanRHall

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amit-momin @jmank88 @prashantkumar1982