Skip to content

RPC sendTransaction: be more liberal in determining and setting last_valid_block_height for skip_preflight case #483

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

Merged
CriesofCarrots merged 2 commits into from
Apr 3, 2024
Merged

RPC sendTransaction: be more liberal in determining and setting last_valid_block_height for skip_preflight case #483

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36e7c45
RPC sendTransaction: if skip_preflight, use processed-commitment Bank…
CriesofCarrots Mar 28, 2024
213ec56
Use nonce retry logic for skip_preflight transactions if blockhash wa…
CriesofCarrots Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions rpc/src/rpc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3645,8 +3645,11 @@ pub mod rpc_full {
let (wire_transaction, unsanitized_tx) =
decode_and_deserialize::<VersionedTransaction>(data, binary_encoding)?;

let preflight_commitment =
preflight_commitment.map(|commitment| CommitmentConfig { commitment });
let preflight_commitment = if skip_preflight {
Some(CommitmentConfig::processed())
Copy link

@t-nelson t-nelson Apr 2, 2024

Choose a reason for hiding this comment

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

on second thought, would confirmed be more reliable here? more likely to pick up ephemeral forks with processed

Copy link
Author

@CriesofCarrots CriesofCarrots Apr 2, 2024

Choose a reason for hiding this comment

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

I can't see a drawback to picking up an ephemeral fork here. This is only being used to look for the recent_blockhash to determine last_valid_block_height. If the blockhash is from a confirmed or finalized block, it should appear in the ancestor tree of the fork.

Copy link
Author

@CriesofCarrots CriesofCarrots Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But I will hold off merging until you reply, in case you've thought of something I missed. @t-nelson

Copy link

@t-nelson t-nelson Apr 3, 2024

Choose a reason for hiding this comment

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

was thinking something like...

C - 1' - 2'    - 3'(STS)
  \ 1  - 2(TX) - 3

Copy link
Author

@CriesofCarrots CriesofCarrots Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I mean, the bigger problem for the tx using a processed blockhash in that example is really that the transaction won't ever land if the leaders are on the same fork-prime as STS.
However, STS will still retry the transaction the fixed number of blocks (300) as per the logic at L3668 (edit) with this patch

Copy link
Author

@CriesofCarrots CriesofCarrots Apr 3, 2024

Choose a reason for hiding this comment

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

...I'm going to merge this as-is, but there's no big risk in tweaking this later.

Copy link

@steveluscher steveluscher Apr 3, 2024

Choose a reason for hiding this comment

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

I'll similarly land solana-foundation/solana-web3.js#2415. If you change your mind on the commitment, let's make the change in the client too!

} else {
preflight_commitment.map(|commitment| CommitmentConfig { commitment })
};
let preflight_bank = &*meta.get_bank_with_config(RpcContextConfig {
commitment: preflight_commitment,
min_context_slot,
Expand All @@ -3662,7 +3665,7 @@ pub mod rpc_full {
let durable_nonce_info = transaction
.get_durable_nonce()
.map(|&pubkey| (pubkey, *transaction.message().recent_blockhash()));
if durable_nonce_info.is_some() {
if durable_nonce_info.is_some() || (skip_preflight && last_valid_block_height == 0) {
// While it uses a defined constant, this last_valid_block_height value is chosen arbitrarily.
// It provides a fallback timeout for durable-nonce transaction retries in case of
// malicious packing of the retry queue. Durable-nonce transactions are otherwise
Expand Down