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

Add preflight checks to sendTransaction RPC method #10338

Merged
merged 7 commits into from
Jun 2, 2020

Conversation

mvines
Copy link
Member

@mvines mvines commented May 30, 2020

sendTransaction blindly dumps the transaction it receives into a TPU. It can do better by:

  1. Verify transaction signatures
  2. Simulating the transaction against the latest max confirmed bank
    and on failure, return an error to the caller. If the RPC node is unhealthy it will refuse to run the simulation

Preflight checks may be disabled if desired with sendTransaction's new config input parameter. By default they are enabled for the convenience of existing API users.

Towards #10336

@mvines mvines force-pushed the preflight branch 5 times, most recently from 6b9fc20 to 6cf67bd Compare June 1, 2020 04:54
@mvines
Copy link
Member Author

mvines commented Jun 1, 2020

@CriesofCarrots - I think this is ready for review. Mostly just playing whack-a-test right now, in some places like program deploy where we (currently) intentionally don't wait for max confirmation between the different phases of deployment and thus the new default sendTransaction preflight checks will fail.

@mvines
Copy link
Member Author

mvines commented Jun 1, 2020

(also CI is being a 🐻)

CriesofCarrots
CriesofCarrots previously approved these changes Jun 1, 2020
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Nice, this looks good to me, pending tests. And (note to self) now that the trusted validators are available in JsonRpcRequestProcessor via health, it will be straightforward to address #9882

core/src/rpc.rs Outdated Show resolved Hide resolved
@mvines
Copy link
Member Author

mvines commented Jun 1, 2020

Added test_rpc_send_transaction_preflight() to cover all the preflight branches

@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #10338 into master will increase coverage by 0.1%.
The diff coverage is 89.7%.

@@            Coverage Diff            @@
##           master   #10338     +/-   ##
=========================================
+ Coverage    81.3%    81.4%   +0.1%     
=========================================
  Files         288      289      +1     
  Lines       67150    67315    +165     
=========================================
+ Hits        54607    54811    +204     
+ Misses      12543    12504     -39     

@mvines
Copy link
Member Author

mvines commented Jun 2, 2020

OMG finally got through CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants