Fix off-by-one bugs with `ScanRange` end bounds
Maximum chain heights are end-inclusive, while `ScanRange` is end-exclusive. Co-authored-by: Daira Hopwood <daira@jacaranda.org>
This commit is contained in:
parent
352e1c709a
commit
e3aeb63e0a
|
@ -817,20 +817,20 @@ pub(crate) fn truncate_to_height<P: consensus::Parameters>(
|
||||||
[u32::from(block_height)],
|
[u32::from(block_height)],
|
||||||
)?;
|
)?;
|
||||||
|
|
||||||
// Delete from the scanning queue any range with a start height greater than or equal to
|
// Delete from the scanning queue any range with a start height greater than the
|
||||||
// the truncation height, and truncate any remaining range by setting the end equal to
|
// truncation height, and then truncate any remaining range by setting the end
|
||||||
// the truncation height.
|
// equal to the truncation height + 1.
|
||||||
conn.execute(
|
conn.execute(
|
||||||
"DELETE FROM scan_queue
|
"DELETE FROM scan_queue
|
||||||
WHERE block_range_start >= :block_height",
|
WHERE block_range_start > :block_height",
|
||||||
named_params![":block_height": u32::from(block_height)],
|
named_params![":block_height": u32::from(block_height)],
|
||||||
)?;
|
)?;
|
||||||
|
|
||||||
conn.execute(
|
conn.execute(
|
||||||
"UPDATE scan_queue
|
"UPDATE scan_queue
|
||||||
SET block_range_end = :block_height
|
SET block_range_end = :end_height
|
||||||
WHERE block_range_end > :block_height",
|
WHERE block_range_end > :end_height",
|
||||||
named_params![":block_height": u32::from(block_height)],
|
named_params![":end_height": u32::from(block_height + 1)],
|
||||||
)?;
|
)?;
|
||||||
|
|
||||||
// Prioritize the range starting at the height we just rewound to for verification
|
// Prioritize the range starting at the height we just rewound to for verification
|
||||||
|
|
|
@ -204,9 +204,15 @@ impl RusqliteMigration for Migration {
|
||||||
)?;
|
)?;
|
||||||
|
|
||||||
if let Some((start, end)) = block_height_extrema(transaction)? {
|
if let Some((start, end)) = block_height_extrema(transaction)? {
|
||||||
|
// `ScanRange` uses an exclusive upper bound.
|
||||||
|
let chain_end = end + 1;
|
||||||
insert_queue_entries(
|
insert_queue_entries(
|
||||||
transaction,
|
transaction,
|
||||||
Some(ScanRange::from_parts(start..end, ScanPriority::Historic)).iter(),
|
Some(ScanRange::from_parts(
|
||||||
|
start..chain_end,
|
||||||
|
ScanPriority::Historic,
|
||||||
|
))
|
||||||
|
.iter(),
|
||||||
)?;
|
)?;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -683,6 +683,9 @@ pub(crate) fn update_chain_tip<P: consensus::Parameters>(
|
||||||
params: &P,
|
params: &P,
|
||||||
new_tip: BlockHeight,
|
new_tip: BlockHeight,
|
||||||
) -> Result<(), SqliteClientError> {
|
) -> Result<(), SqliteClientError> {
|
||||||
|
// `ScanRange` uses an exclusive upper bound.
|
||||||
|
let chain_end = new_tip + 1;
|
||||||
|
|
||||||
// Read the maximum height from the shards table.
|
// Read the maximum height from the shards table.
|
||||||
let shard_start_height = conn.query_row(
|
let shard_start_height = conn.query_row(
|
||||||
"SELECT MAX(subtree_end_height)
|
"SELECT MAX(subtree_end_height)
|
||||||
|
@ -694,8 +697,8 @@ pub(crate) fn update_chain_tip<P: consensus::Parameters>(
|
||||||
// Create a scanning range for the fragment of the last shard leading up to new tip.
|
// Create a scanning range for the fragment of the last shard leading up to new tip.
|
||||||
// However, only do so if the start of the shard is at a stable height.
|
// However, only do so if the start of the shard is at a stable height.
|
||||||
let shard_entry = shard_start_height
|
let shard_entry = shard_start_height
|
||||||
.filter(|h| h < &new_tip)
|
.filter(|h| h < &chain_end)
|
||||||
.map(|h| ScanRange::from_parts(h..new_tip, ScanPriority::ChainTip));
|
.map(|h| ScanRange::from_parts(h..chain_end, ScanPriority::ChainTip));
|
||||||
|
|
||||||
// Create scanning ranges to either validate potentially invalid blocks at the wallet's view
|
// Create scanning ranges to either validate potentially invalid blocks at the wallet's view
|
||||||
// of the chain tip,
|
// of the chain tip,
|
||||||
|
@ -703,7 +706,7 @@ pub(crate) fn update_chain_tip<P: consensus::Parameters>(
|
||||||
// If we don't have shard metadata, this means we're doing linear scanning, so create a
|
// If we don't have shard metadata, this means we're doing linear scanning, so create a
|
||||||
// scan range from the prior tip to the current tip with `Historic` priority.
|
// scan range from the prior tip to the current tip with `Historic` priority.
|
||||||
if shard_entry.is_none() {
|
if shard_entry.is_none() {
|
||||||
ScanRange::from_parts(prior_tip..new_tip, ScanPriority::Historic)
|
ScanRange::from_parts(prior_tip..chain_end, ScanPriority::Historic)
|
||||||
} else {
|
} else {
|
||||||
// Determine the height to which we expect blocks retrieved from the block source to be stable
|
// Determine the height to which we expect blocks retrieved from the block source to be stable
|
||||||
// and not subject to being reorg'ed.
|
// and not subject to being reorg'ed.
|
||||||
|
@ -719,7 +722,7 @@ pub(crate) fn update_chain_tip<P: consensus::Parameters>(
|
||||||
// tip range has been completely checked and any required rewinds have been performed.
|
// tip range has been completely checked and any required rewinds have been performed.
|
||||||
if prior_tip >= stable_height {
|
if prior_tip >= stable_height {
|
||||||
// This may overlap the `shard_entry` range and if so will be coalesced with it.
|
// This may overlap the `shard_entry` range and if so will be coalesced with it.
|
||||||
ScanRange::from_parts(prior_tip..new_tip, ScanPriority::ChainTip)
|
ScanRange::from_parts(prior_tip..chain_end, ScanPriority::ChainTip)
|
||||||
} else {
|
} else {
|
||||||
// The prior tip is in the range that we now expect to be stable, so we need to verify
|
// The prior tip is in the range that we now expect to be stable, so we need to verify
|
||||||
// and advance it up to at most the stable height. The shard entry will then cover
|
// and advance it up to at most the stable height. The shard entry will then cover
|
||||||
|
@ -754,7 +757,7 @@ pub(crate) fn update_chain_tip<P: consensus::Parameters>(
|
||||||
// activation.
|
// activation.
|
||||||
if let Some(sapling_activation) = params.activation_height(NetworkUpgrade::Sapling) {
|
if let Some(sapling_activation) = params.activation_height(NetworkUpgrade::Sapling) {
|
||||||
let scan_range =
|
let scan_range =
|
||||||
ScanRange::from_parts(sapling_activation..new_tip, ScanPriority::Historic);
|
ScanRange::from_parts(sapling_activation..chain_end, ScanPriority::Historic);
|
||||||
insert_queue_entries(conn, Some(scan_range).iter())?;
|
insert_queue_entries(conn, Some(scan_range).iter())?;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
@ -1082,7 +1085,7 @@ mod tests {
|
||||||
assert_matches!(
|
assert_matches!(
|
||||||
db_data.suggest_scan_ranges(),
|
db_data.suggest_scan_ranges(),
|
||||||
Ok(scan_ranges) if scan_ranges == vec![
|
Ok(scan_ranges) if scan_ranges == vec![
|
||||||
scan_range((sap_active + 320)..(sap_active + 340), ChainTip),
|
scan_range((sap_active + 320)..(sap_active + 341), ChainTip),
|
||||||
scan_range((sap_active + 300)..(sap_active + 310), ChainTip)
|
scan_range((sap_active + 300)..(sap_active + 310), ChainTip)
|
||||||
]
|
]
|
||||||
);
|
);
|
||||||
|
@ -1099,7 +1102,7 @@ mod tests {
|
||||||
db_data.suggest_scan_ranges(),
|
db_data.suggest_scan_ranges(),
|
||||||
Ok(scan_ranges) if scan_ranges == vec![
|
Ok(scan_ranges) if scan_ranges == vec![
|
||||||
scan_range((sap_active + 319)..(sap_active + 329), Verify),
|
scan_range((sap_active + 319)..(sap_active + 329), Verify),
|
||||||
scan_range((sap_active + 329)..(sap_active + 450), ChainTip),
|
scan_range((sap_active + 329)..(sap_active + 451), ChainTip),
|
||||||
scan_range((sap_active + 300)..(sap_active + 310), ChainTip)
|
scan_range((sap_active + 300)..(sap_active + 310), ChainTip)
|
||||||
]
|
]
|
||||||
);
|
);
|
||||||
|
|
Loading…
Reference in New Issue