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.

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

feat: a nonce-based transaction confirmation strategy for web3.js #25839

Merged
merged 8 commits into from Nov 29, 2022
Merged

feat: a nonce-based transaction confirmation strategy for web3.js #25839

merged 8 commits into from Nov 29, 2022

Conversation

steveluscher
Copy link
Contributor

@steveluscher steveluscher commented Jun 8, 2022

Usage

const tx = new Transaction({nonceInfo});
const signature = connection.sendTransaction(tx, [/* ... */]);
await connection.confirmTransaction({
  minContextSlot, // A slot where the nonce was known to be the candidate value.
  nonceAccountPubkey,  // The account where we can expect to find the latest nonce value.
  nonceValue: nonceInfo.nonce,  // Whatever nonce value the `tx` contained when it was signed.
  signature,
});

Closes #25303.

@steveluscher steveluscher changed the title Nonce based transaction confirmation feat: a nonce-based transaction confirmation strategy for web3.js Jun 8, 2022
@steveluscher steveluscher marked this pull request as ready for review June 8, 2022 22:19
@steveluscher steveluscher requested a review from jstarry June 8, 2022 22:19
@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #25839 (b03c38f) into master (04789ca) will decrease coverage by 0.2%.
The diff coverage is 69.7%.

@@            Coverage Diff            @@
##           master   #25839     +/-   ##
=========================================
- Coverage    76.8%    76.5%   -0.3%     
=========================================
  Files          55       55             
  Lines        2986     3084     +98     
  Branches      426      454     +28     
=========================================
+ Hits         2294     2362     +68     
- Misses        544      564     +20     
- Partials      148      158     +10     

web3.js/src/nonce-account.ts Outdated Show resolved Hide resolved
web3.js/src/transaction.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
@steveluscher
Copy link
Contributor Author

steveluscher commented Jun 29, 2022

Alright! This is ready for you again @jstarry. All the new commits at the bottom are part of #26296.

@steveluscher steveluscher added enhancement New feature or request javascript Pull requests that update Javascript code labels Jun 29, 2022
Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Can you PR the non-confirmation changes separately so that we can get those in more quickly and break up this large PR?

web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
Comment on lines 3147 to 3149
while (
true // eslint-disable-line no-constant-condition
) {
Copy link
Member

Choose a reason for hiding this comment

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

Could this be refactored to while (!done) and then each loop we either sleep or get the current nonce value? That way we don't need so many done checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There’s a done check after each async operation, because it’s at that point we need to check to make sure a confirmation didn’t come in while we were waiting for the async op to finish.

For instance, we want to check for done after the sleep, so that we don’t waste time on another nonce fetch for no reason. Putting the done check at the loop level would make this impossible.

Copy link
Member

@jstarry jstarry Jul 8, 2022

Choose a reason for hiding this comment

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

Putting the done check at the loop level would make this impossible

My suggestion was to make the loop only run one async operation per loop so that done is still called immediately after. So it's possible, but I suppose it's less obvious so this isn't a big deal.

        let readyToPoll = false;
        let currentBlockHeight = await checkBlockHeight();
        while (!done && currentBlockHeight <= lastValidBlockHeight) {
          if (readyToPoll) {
            currentBlockHeight = await checkBlockHeight();
          } else {
            await sleep(1000);
          }
        }

        if (!done) {
          resolve({ __type: TransactionStatus.BLOCKHEIGHT_EXCEEDED });
        }

web3.js/src/nonce-account.ts Outdated Show resolved Hide resolved
web3.js/src/transaction.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

I’ll massage this further when I get back to a computer!

@steveluscher steveluscher requested a review from jstarry July 2, 2022 21:42
@steveluscher
Copy link
Contributor Author

steveluscher commented Jul 2, 2022

Alright, done! How about this, instead of separate PRs, I've reduced this PR to 5 commits that each do exactly one thing. You can review each as though it was it's own PR.

Because it's 2022, and GitHub still doesn't have stacked PRs.

image

web3.js/src/transaction.ts Outdated Show resolved Hide resolved
web3.js/src/transaction.ts Outdated Show resolved Hide resolved
web3.js/src/transaction.ts Outdated Show resolved Hide resolved
web3.js/src/transaction.ts Outdated Show resolved Hide resolved
web3.js/src/transaction.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Outdated Show resolved Hide resolved
web3.js/src/connection.ts Show resolved Hide resolved
web3.js/src/connection.ts Show resolved Hide resolved
web3.js/src/connection.ts Show resolved Hide resolved
web3.js/src/connection.ts Show resolved Hide resolved
@steveluscher
Copy link
Contributor Author

Updated!

  • Rebased
  • Updated/added code comments
  • Renamed NonceBasedTransactionConfirmationStrategy to DurableNonceTransactionConfirmationStrategy
  • Fixed spelling of Exceedance
  • Fixed spelling of BlockHeight
  • Removed signature from… method signature.
  • Created object return datatype for getTransactionConfirmationPromise
  • Eliminated INVALID_NONCE_ERROR_SENTINEL
  • Added comment pointing out exhaustive switch.

@stale
Copy link

stale bot commented Jul 31, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 31, 2022
@steveluscher steveluscher removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 31, 2022
web3.js/src/connection.ts Outdated Show resolved Hide resolved
@jstarry
Copy link
Member

jstarry commented Aug 12, 2022

Apologies for dropping off on the review of this one. The main points to address are:

  1. The logic for double checking that a nonce transaction hasn't been processed isn't quite right: feat: a nonce-based transaction confirmation strategy for web3.js #25839 (comment) and feat: a nonce-based transaction confirmation strategy for web3.js #25839 (comment)
  2. Looks like this branch should check for the existence of minContextSlot before referencing it: feat: a nonce-based transaction confirmation strategy for web3.js #25839 (comment)

@steveluscher
Copy link
Contributor Author

Alright; rebased and shipping this, at long last. If it ends up sucking we can revert it. If we find there are improvements to make, we can follow up with those improvements!

@steveluscher steveluscher merged commit 7646521 into solana-labs:master Nov 29, 2022
@steveluscher steveluscher deleted the nonce-based-transaction-confirmation branch November 29, 2022 06:46
gnapoli23 pushed a commit to gnapoli23/solana that referenced this pull request Dec 16, 2022
…lana-labs#25839)

* feat: you can now construct a `Transaction` with durable nonce information

* chore: refactor confirmation logic so that each strategy gets its own method

* feat: `geNonce` now accepts a `minContextSlot` param

* feat: a nonce-based transaction confirmation strategy

* feat: add nonce confirmation strategy to send-and-confirm helpers

* fix: nits from July 8 review

* Use Typescript narrowing to determine which strategy to use

* Double check the signature confirmation against the slot in which the nonce was discovered to have advanced
nickfrosty pushed a commit to nickfrosty/solana that referenced this pull request Jan 4, 2023
…lana-labs#25839)

* feat: you can now construct a `Transaction` with durable nonce information

* chore: refactor confirmation logic so that each strategy gets its own method

* feat: `geNonce` now accepts a `minContextSlot` param

* feat: a nonce-based transaction confirmation strategy

* feat: add nonce confirmation strategy to send-and-confirm helpers

* fix: nits from July 8 review

* Use Typescript narrowing to determine which strategy to use

* Double check the signature confirmation against the slot in which the nonce was discovered to have advanced
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

web3.js: Implement a nonce-based transaction confirmation strategy
2 participants