fix: fix transaction confirmation logic

[marcnjaramillo]

Problem

Currently, one issue is that sendAndConfirmTransaction relies on confirmTransaction to determine whether a transaction has expired. This is an issue because confirmTransaction simply waits for 60 seconds rather than using the lastValidBlockHeight associated with a transaction's blockhash.

Summary of Changes

This PR attempts to move the transaction confirmation logic closer to a correct state by allowing the Transaction constructor to accept the entire results object from calling getLatestBlockhash rather than only the blockhash. This requires creating a new TransactionCtorFields__DEPRECATED to continue allowing the current implementation to be used while updating TransactionCtorFields to accept the updated values.

Because of the relationship between transactions and messages, this change also required a change to Message to likewise accept the lastValidBlockHeight that results from calling getLatestBlockhash.

Finally, we have updated sendAndConfirmTransaction to no longer use the current confirmTransaction. Instead, it compares the current blockHeight to lastValidBlockHeight while waiting for the transaction to be confirmed.

Fixes #23949
Fixes #24730
Closes #24757

[steveluscher]

I’ll look at this soon, thanks for your patience!

[steveluscher]

This is going to be so great when it's fixed. Thanks for working on it! We have a good deal of ground to cover here before this is fast, light, and reliable enough for production, but start with these comments and we'll get there soon!

Pull request has been modified.

[steveluscher]

Great changes; keep going!

[steveluscher]

Keep on going!

Pull request has been modified.

[marcnjaramillo]

Hey Steve, left some comments to give updates on what I've been able to do. Let me know what you think!

[steveluscher]

So close! I can see it coming together.

It's time to start writing tests for this stuff. Are you familiar with writing tests in this codebase, or should we hop on a call to talk through it?

[steveluscher]

You haven't rebased atop the main branch in a really long time. You might want to git pull --rebase git@github.com:solana-labs/solana.git master sooner than later. I just tried it and your PR applied cleanly.

[steveluscher]

@stellaw1, when you rebase this code atop the main branch again, you'll discover a build error caused by the fact that these two definitions snuck in between your constructor overloads and the constructor implementation.

That's what's causing the build errors on this PR. Just move them out of the way so that all 4 constructor signatures come one after another.

[marcnjaramillo]

You haven't rebased atop the main branch in a really long time. You might want to git pull --rebase git@github.com:solana-labs/solana.git master sooner than later. I just tried it and your PR applied cleanly.

Yeah, I'm a bit embarrassed that I hadn't rebased in so long. When Stella pushed her changes I did a rebase to get everything up to date. I try to rebase more frequently, but I just lost track of time and forgot about it. I'll make sure to keep on it now!

Update: I evidently did not rebase everything the way I thought I had. I thought I had gotten my local master branch up to date, and then updated my feature branch from there.

[marcnjaramillo]

So close! I can see it coming together.

It's time to start writing tests for this stuff. Are you familiar with writing tests in this codebase, or should we hop on a call to talk through it?

I haven't done much with tests in this codebase, so if you've got some time to get on a call I would appreciate it.

Pull request has been modified.

[marcnjaramillo]

The test suite passes, but the builds are all failing because of this:

> @solana/web3.js@0.0.0-development doc
> set -ex; typedoc --treatWarningsAsErrors

+ typedoc --treatWarningsAsErrors
Warning: BlockheightBasedTransactionConfimationStrategy, defined at src/connection.ts:287, is referenced by Connection.confirmTransaction.confirmTransaction.strategy but not included in the documentation.
ERROR: "doc" exited with 4.

I've seen this once before but I have no idea how to fix it.

[steveluscher]

That build failure just means that you have to write documentation (in the form of code comments) above BlockheightBasedTransactionConfimationStrategy so that typedoc can build it into the documentation website for you! Write something that explains when to use that strategy, and a little bit about how it works.

[steveluscher]

Love it! Just a few quick comments before I grab a bite to eat. I'll finish the review after that!

[steveluscher]

Nice! Just some tidy up. Let me know if you want help with the tests; I'm happy to check this PR out and give it a go.

Pull request has been modified.

[steveluscher]

Yay! Let's ship this next week!

Thank you for all of your relentless work on this. Your work here is going to help people get notices of failed transactions sooner so that they can take action faster. This is going to ripple all the way through the Solana ecosystem, ultimately increasing the number of successful transactions, and joy using products and services.

I'll do a once over of the tests if you want to fix the few typos, and then I think we're gtg!

[steveluscher]

And @stellaw1, the same goes for you. Thank you for your contributions here, and for being part of our awesome developer community!

Pull request has been modified.

[steveluscher]

Here, I fixed up the tests, and made some minor tweaks to let/const bindings. What do you think of these? Feel free to pull them in to this PR or commit them under your account: https://github.com/steveluscher/solana/commits/fix-transaction-confirmation

[steveluscher]

I made a couple of fixes on https://github.com/steveluscher/solana/commits/fix-transaction-confirmation (I still can't seem to push updates to your branch) but one thing we have to do before shipping this is to make sure that your new tests run with yarn test:live-with-test-validator. Right now they don't.

I think the right thing to do here is to simply make them not run when process.env.TEST_LIVE; this is probably reasonable because it's more interesting how that code performs under a very specific, simulated scenario, than it is under a realtime scenario.

[marcnjaramillo]

I still can't seem to push updates to your branch

I think the only way to fix this is to close this PR and open a new PR from my own personal branch. For whatever reason, I don't have an option to allow maintainer edits on a PR from the MLH-Fellowship fork. In the meantime I'll go ahead and merge your changes into this branch. In the future I'll make PRs from my own branch.

I think the right thing to do here is to simply make them not run when process.env.TEST_LIVE; this is probably reasonable because it's more interesting how that code performs under a very specific, simulated scenario, than it is under a realtime scenario.

This makes sense. I'll also make this change.

[codecov]

Codecov Report

Merging #24211 (7745581) into master (69a0ff9) will decrease coverage by 6.7%.
The diff coverage is n/a.

❗ Current head 7745581 differs from pull request most recent head 89b9e01. Consider uploading reports for the commit 89b9e01 to get more accurate results

@@             Coverage Diff             @@
##           master   #24211       +/-   ##
===========================================
- Coverage    82.0%    75.3%     -6.8%     
===========================================
  Files         598       38      -560     
  Lines      165882     2291   -163591     
  Branches        0      333      +333     
===========================================
- Hits       136125     1726   -134399     
+ Misses      29757      447    -29310     
- Partials        0      118      +118     

Pull request has been modified.

[steveluscher]

Wait, I'm confused. The Message type can have latestBlockhash on it. If present, then you have all you need to reconstruct a Transaction with a lastValidBlockHash.

Instead of changing the method sig here, can you just inspect message, looking for latestBlockhash.lastValidBlockHeight, and on the compileMessage side make sure that you write out a latestBlockhash?

[marcnjaramillo]

That was part of this PR, but we didn't finish it because we realized it was going to be more involved. If you look down further in Message to the from method, we don't do anything with lastValidBlockHeight. So if we include latestBlockhash in a constructed message, lastValidBlockHeight is undefined.