pack/resolv: fix transaction expiration logic #7128
Conversation
| uint actual_consumed_cus; /* non_execution_cus+real execution CUs+real account data cus. PoH reads this for block CU counting. */ | ||
| } bank_cu; /* Populated by bank. */ | ||
| ulong blockhash_slot; /* Slot provided by resolv tile when txn arrives at the pack tile. Used when txn is in extra storage in pack. */ | ||
| ulong block_height; /* Block height provided by resolv tile when txn arrives at the pack tile. Used when txn is in extra storage in pack. */ |
There was a problem hiding this comment.
This is the blockhash_block_height, not block_height
There was a problem hiding this comment.
Right, I missed that because this field seems only used when FD_PACK_USE_EXTRA_STORAGE is on.
| if( FD_UNLIKELY( (ctx->leader_slot==ULONG_MAX) & (sig>ctx->highest_observed_slot) ) ) { | ||
| /* Using the resolv tile's knowledge of the current slot is a bit | ||
| if( FD_UNLIKELY( (ctx->leader_slot==ULONG_MAX) & (sig>ctx->root_block_height) ) ) { | ||
| /* Using the resolv tile's knowledge of the current last root block height is a bit |
There was a problem hiding this comment.
Ok. Comments are 72-wide but not the code ?
| happen if we are getting transactions. */ | ||
| ctx->highest_observed_slot = sig; | ||
| ulong exp_cnt = fd_pack_expire_before( ctx->pack, fd_ulong_max( ctx->highest_observed_slot, TRANSACTION_LIFETIME_SLOTS )-TRANSACTION_LIFETIME_SLOTS ); | ||
| ctx->root_block_height = sig; |
There was a problem hiding this comment.
This isn't the root block height though? It's the block height of the referenced blockhash, I don't think this logic works
There was a problem hiding this comment.
The idea is to pass the root block height as the sig, and use it to discard already buffered txs, and the reference (= blockhash) block height of the sent tx in the reference_block_height of the fd_txn_m.
Why do you think it won't work ?
mmcgee-jump
left a comment
mmcgee-jump
left a comment
There was a problem hiding this comment.
Thank you for the contribution!
| ulong deleted; | ||
| long insert_duration = -fd_tickcount(); | ||
| int result = fd_pack_insert_txn_fini( ctx->pack, ctx->cur_spot, blockhash_slot, &deleted ); | ||
| int result = fd_pack_insert_txn_fini( ctx->pack, ctx->cur_spot, block_height, &deleted ); |
There was a problem hiding this comment.
There is a problem here though. fd_pack_insert_txn_fini should be called with the reference block hash, not the root block hash passed using sig.
Couldn't we remove this third argument and use ctx->cur_spot->txnp->reference_block_height instead, ctx->cur_sport being the second argument ?
2 participants
Currently, Firedancer discards transactions before sending them to bank tile by considering the slot difference between the reference block and the current slot. This is deviating from the protocol and causes transactions to be wrongly ignored if at least 1 slot was skipped during the last 151 slots. The difference in block heights should be used instead.
Fixes #7052
resolvnow maps block hashes to block heights instead of to slots.resolvforwards topackthe txs that have a reference block height earlier or equal to 151 before last root block height, instead of the last completed slot.packnow receives fromresolvthe last root block height as message signature instead of the last observed slot. Only the root block has a finalized block height that we can base on to discard expired txs. When a transaction is received with a new root block height, it discards all buffered txs referencing a slot with a block height earlier than 151 before.packdiscards all txs based on leader slot block height.replay->resolv"root advanced" message.bank->resolv"rooted bank" message.bank->resolv"blockhash completed" message.In
resolv, I kept the last completed block height to initialize the reference block height when a transaction is received with an unknown blockhash, but I'm not sure it is useful, since it will be replaced byflushing_block_heightwhen it leaves the stash.completed_block_heightis probably removable.The changes to agave
Shall I open a PR against some branch of firedancer agave repo ?
Tested with frankendancer. Not with full firedancer.