Skip to content
Merged
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,16 @@ where
.withdrawal_transactions_per_block_limit,
"No queued withdrawal documents found to pool into transactions"
);
// Reading every withdrawal document the chain ever produced, only to
// count them for a log line. Withdrawal documents are never removed,
// so this grows without bound with chain history — measured at 4.8 ms
// a block by height 200,000 on mainnet, and it ran on every block
// that had nothing queued, which is nearly all of them. Do it only
// when the line it feeds will actually be emitted.
if !tracing::enabled!(tracing::Level::DEBUG) {
return Ok(());
}

let all_documents = self
.drive
.fetch_oldest_withdrawal_documents(transaction, platform_version)?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔴 Blocking: Do not change active versioned block-execution behavior in place

pool_withdrawals_into_transactions_queue_v1 is selected from protocol v5 onward, including v14 through DRIVE_ABCI_METHOD_VERSIONS_V10, while the v0 implementation can also delegate to it. This therefore changes behavior for already-defined protocol versions. More importantly, fetch_oldest_withdrawal_documents calls the fallible query_documents path and previously propagated its error unconditionally; after this guard, the same block and state can return Ok(()) on a node without DEBUG tracing while a DEBUG-enabled node or older binary still executes the query and can fail. The successful app-hash replay confirms the normal path but does not cover this changed failure behavior. Keep v0/v1 frozen, introduce a v2 implementation selected at a protocol activation boundary, and make its purely diagnostic query non-fatal so tracing configuration cannot affect block-execution success.

source: ['claude']

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Partially addressed in efd398de: the diagnostic fetch is no longer fatal. A Drive error in fetch_oldest_withdrawal_documents is logged at DEBUG and the method returns Ok(()), so block outcome no longer depends on the log level. Verified on head cf5e8e4a.

The question of whether this needs a v2 method version rather than an in-place edit to pool_withdrawals_into_transactions_queue_v1 is left for the maintainer to decide; this thread stays open for that.


🤖 Posted autonomously by Claude on behalf of pasta.

Expand All @@ -57,7 +67,7 @@ where
height = block_info.height,
"No withdrawal documents found at all"
);
} else if tracing::enabled!(tracing::Level::DEBUG) {
} else {
// Count documents by status
let queued_count = all_documents
.get(&(withdrawals_contract::WithdrawalStatus::QUEUED as u8))
Expand Down
Loading