fix: select_transactions_records edge case#2285
Conversation
sergerad
changed the title
| "transaction records for block {block_num} exceed the maximum response payload size and \ | ||
| cannot be paginated" |
There was a problem hiding this comment.
We should help the caller understand what they can do in this situation.
| "transaction records for block {block_num} exceed the maximum response payload size and \ | |
| cannot be paginated" | |
| "transactions for block {block_num} would exceed maximum response size, \ | |
| use a stricter filter to reduce the number of transactions returned" |
There was a problem hiding this comment.
I'm finding this function quite confusing. The fix works - I think - but given the function layout I'm a bit worried there's something I'm overlooking.
However I think what I want to suggest isn't possible because the table is without rowid.
Maybe @kkovaacs can offer some insight.
What I want to do is roughly:
// Find the first applicable row.
let mut row_start = db.select(
"SELECT rowid FROM transactions WHERE <account+block start filter>"
);
// The last completed full block page index ito tx records.
let mut iblock = 0usize;
let mut records = Vec::new();
let mut last_block = BlockNumber::MAX;
loop {
let chunk = db.select(
"SELECT tx, rowid, block FROM transactions WHERE <filter> AND rowid >= row_start LIMIT 1000 ORDER BY rowid"
);
for row in chunk {
if records.size() + row > capacity {
break;
}
row_start = row.rowid;
if row.block != last_block {
last_block = row.block;
iblock = records.len();
}
records.push(row.tx);
}
}
// Truncate to the last full block.
records = records[..iblock];
Ok(records);| // Set to the block number of the first transaction that did not fit within the payload cap. | ||
| // This is the explicit "we truncated" signal; the accumulated byte total cannot be used as a | ||
| // proxy, since a transaction can fail to fit while `total_size` is still below the cap. | ||
| let mut truncated_at_block: Option<i64> = None; |
There was a problem hiding this comment.
I would suggest an alternate approach, like I outlined in https://github.com/0xMiden/node/pull/2285/changes#r3474272591.
| let complete_transactions: Vec<_> = all_transactions | ||
| .into_iter() | ||
| .take_while(|row| row.block_num < truncation_block) | ||
| .collect(); |
There was a problem hiding this comment.
If you track the last index in addition to the last completed block, then you can instead all_transactions.truncate(ilast_completed).
| "transaction records for block {block_num} exceed the maximum response payload size and \ | ||
| cannot be paginated" | ||
| )] | ||
| TransactionPageExceedsPayloadLimit { block_num: BlockNumber }, |
There was a problem hiding this comment.
This new error case is mapped to a gRPC internal error, while in reality it should probably be Status::resource_exhausted() or something like that.
3 participants
Summary
select_transactions_recordspaginates by payload size, but it inferred truncation fromtotal_size >= cap, which missed the common case where a transaction fails to fit while the running total is still below the cap — making a partial page look complete.The fix tracks an explicit "transaction didn't fit" signal so a truncated page reports the last complete block as the cursor, and errors when a single block exceeds the payload cap.
Changelog