Skip to content

Commit 577fe3d

Browse files
committed
fix(drive-abci): rebuild a ranked envelope that loses its race with a commit
A ranked page is committed to by an envelope built from several independent storage reads, and those are not isolated from a concurrent block commit. A commit landing inside that window leaves the ancestor chain unreconcilable, grovedb rejects it, and the request fails. Before this surface read through the prover, only `prove = true` was exposed; now the default read path is too. `query::service` already re-runs a query whose committed height moved, which absorbs almost all of it, but `finalize_block` makes a commit visible before publishing that height, and a request finishing inside that gap is not retried. Rebuild the envelope instead of returning: the condition is transient and a fresh envelope over settled state resolves it. Deliberately narrow. Only the chain mismatch is retried, bounded at two extra attempts (~400 us total), and genuine corruption produces the same rejection every time and so still surfaces — one envelope later, with a `warn` that says a burst under load is the race while a persistent or unloaded occurrence is not. Also covers three defensive paths that had no tests: the shared entry decoder's axis-shape and `k` guards, unreachable through grovedb today and therefore worth pinning precisely because nothing else can reach them, and the chain-mismatch detector's narrowness. Reviewed-by: CodeRabbit (retry suggestion adopted; its exact-match suggestion for the empty-tree marker was rejected — grovedb wraps merk's constant in its own prefix, so exact matching would stop the mapper from ever firing)
1 parent f432daa commit 577fe3d

3 files changed

Lines changed: 186 additions & 29 deletions

File tree

  • packages

packages/rs-drive-abci/src/query/document_query/v1/mod.rs

Lines changed: 74 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1317,26 +1317,45 @@ impl<C> Platform<C> {
13171317
document_type_name, contract_id
13181318
))));
13191319

1320-
let drive_request = DocumentRankedRequest {
1321-
contract: contract_ref,
1322-
document_type,
1323-
group_by: &group_by,
1324-
select,
1325-
having: &having,
1326-
order_by: &order_clauses,
1327-
where_clauses: &where_clauses,
1328-
limit,
1329-
offset,
1330-
has_start_at: start.is_some(),
1331-
prove,
1332-
};
1320+
// A ranked page is committed to by an envelope built from several
1321+
// independent storage reads, which are not isolated from a
1322+
// concurrent block commit. A commit landing inside that window
1323+
// leaves the envelope's ancestor chain unreconcilable and grovedb
1324+
// rejects it — a transient condition that a fresh envelope over
1325+
// settled state resolves, so it is retried here rather than
1326+
// returned. `query::service` already re-runs a query whose
1327+
// committed height moved, but there is a window between a commit
1328+
// becoming visible and that height being published where it does
1329+
// not, and this closes it.
1330+
//
1331+
// Deliberately narrow: only the chain mismatch is retried, and
1332+
// only a bounded number of times. Genuine corruption produces the
1333+
// same rejection on every attempt and so still surfaces, one
1334+
// envelope later.
1335+
let mut attempts_left = RANKED_CHAIN_MISMATCH_RETRIES;
1336+
let drive_response = loop {
1337+
// Rebuilt per attempt rather than cloned: every field is a
1338+
// borrow of state that outlives the loop, plus the request's
1339+
// one owned value.
1340+
let drive_request = DocumentRankedRequest {
1341+
contract: contract_ref,
1342+
document_type,
1343+
group_by: &group_by,
1344+
select: select.clone(),
1345+
having: &having,
1346+
order_by: &order_clauses,
1347+
where_clauses: &where_clauses,
1348+
limit,
1349+
offset,
1350+
has_start_at: start.is_some(),
1351+
prove,
1352+
};
13331353

1334-
let drive_response =
13351354
match self
13361355
.drive
13371356
.execute_document_ranked_request(drive_request, None, platform_version)
13381357
{
1339-
Ok(r) => r,
1358+
Ok(r) => break r,
13401359
Err(drive::error::Error::Query(qe)) => {
13411360
return Ok(QueryValidationResult::new_with_error(QueryError::Query(qe)));
13421361
}
@@ -1345,11 +1364,16 @@ impl<C> Platform<C> {
13451364
return Ok(QueryValidationResult::new_with_error(rejection));
13461365
}
13471366
None => {
1367+
if is_ranked_chain_mismatch(&e) && attempts_left > 0 {
1368+
attempts_left -= 1;
1369+
continue;
1370+
}
13481371
annotate_ranked_chain_mismatch(&e);
13491372
return Err(e.into());
13501373
}
13511374
},
1352-
};
1375+
}
1376+
};
13531377

13541378
let response = match drive_response {
13551379
DocumentRankedResponse::Entries(page) => GetDocumentsResponseV1 {
@@ -1471,6 +1495,35 @@ fn into_v1_ranked_entry(e: DriveRankedEntry) -> RankedEntry {
14711495
/// narrow: any other `CorruptedData` still propagates as an internal
14721496
/// error, because for every other cause that classification is
14731497
/// correct.
1498+
/// How many times a ranked request is rebuilt when its envelope loses the
1499+
/// race against a block commit.
1500+
///
1501+
/// The condition needs a commit to land inside one envelope's read window,
1502+
/// which takes microseconds, so a single retry over settled state is
1503+
/// already overwhelmingly likely to succeed; two bounds the cost at three
1504+
/// envelopes (~400 µs) for a request that would otherwise have failed.
1505+
/// Raising it would trade real work against a vanishing tail.
1506+
const RANKED_CHAIN_MISMATCH_RETRIES: u8 = 2;
1507+
1508+
/// Whether a drive error is the ancestor-chain reconciliation failure that
1509+
/// a concurrent commit produces.
1510+
///
1511+
/// Detected by variant plus marker substring rather than by a typed error,
1512+
/// for the same reason [`empty_ranking_proof_rejection`] is: grovedb
1513+
/// flattens the failure into a `CorruptedData(String)` at the indexed-axis
1514+
/// proof boundary. "chain mismatch" is the substring both of grovedb's
1515+
/// reconciliation errors carry (the deepest layer's and the intermediate
1516+
/// ancestors').
1517+
fn is_ranked_chain_mismatch(error: &drive::error::Error) -> bool {
1518+
let drive::error::Error::GroveDB(grove_error) = error else {
1519+
return false;
1520+
};
1521+
let drive::query::GroveError::CorruptedData(message) = grove_error.as_ref() else {
1522+
return false;
1523+
};
1524+
message.contains("chain mismatch")
1525+
}
1526+
14741527
/// Name the benign cause of a ranked chain-mismatch error in the log,
14751528
/// without reclassifying it.
14761529
///
@@ -1493,20 +1546,14 @@ fn into_v1_ranked_entry(e: DriveRankedEntry) -> RankedEntry {
14931546
/// exposure; unproved reads share it now that they are served from the
14941547
/// same envelope.
14951548
fn annotate_ranked_chain_mismatch(error: &drive::error::Error) {
1496-
let drive::error::Error::GroveDB(grove_error) = error else {
1497-
return;
1498-
};
1499-
let drive::query::GroveError::CorruptedData(message) = grove_error.as_ref() else {
1500-
return;
1501-
};
1502-
if !message.contains("chain mismatch") {
1549+
if !is_ranked_chain_mismatch(error) {
15031550
return;
15041551
}
15051552
tracing::warn!(
1506-
error = message.as_str(),
1507-
"ranked query failed to reconcile its proof's ancestor chain; an isolated \
1508-
occurrence under load is a query racing a block commit rather than corrupted \
1509-
state, but repeated occurrences — especially without load — are not"
1553+
error = %error,
1554+
"ranked query failed to reconcile its proof's ancestor chain on every attempt; \
1555+
a burst under load is queries racing block commits, but a persistent or \
1556+
unloaded occurrence is not and should be treated as suspected corruption"
15101557
);
15111558
}
15121559

packages/rs-drive-abci/src/query/document_query/v1/tests.rs

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3089,4 +3089,48 @@ mod ranked_tests {
30893089
"the marker string alone must not reclassify a non-grovedb error"
30903090
);
30913091
}
3092+
3093+
/// The chain-mismatch detector decides whether a ranked request is
3094+
/// rebuilt or surfaced, so its narrowness is the whole safety property:
3095+
/// retrying the wrong error class would spend three envelopes on a
3096+
/// failure that cannot improve, and — worse — retrying *everything*
3097+
/// would turn a genuine fault into three of them.
3098+
///
3099+
/// The message it matches is grovedb's, wrapped: the reconciliation
3100+
/// failure arrives as `CorruptedData` with the marker embedded in
3101+
/// grovedb's own prefix, never as the bare marker, which is why this
3102+
/// matches a substring rather than the whole string.
3103+
#[test]
3104+
fn only_a_grovedb_chain_mismatch_is_retried() {
3105+
use drive::error::Error as DriveError;
3106+
use drive::query::GroveError;
3107+
3108+
let wrapped = "indexed-axis paginated proof: intermediate layer at depth 2 chain \
3109+
mismatch — parent recorded value_hash ab, computed cd";
3110+
assert!(
3111+
is_ranked_chain_mismatch(&DriveError::GroveDB(Box::new(GroveError::CorruptedData(
3112+
wrapped.to_string()
3113+
)))),
3114+
"the reconciliation failure must be recognised inside grovedb's wrapper text"
3115+
);
3116+
3117+
// Everything else is surfaced on the first attempt.
3118+
for other in [
3119+
GroveError::CorruptedData("Cannot create proof for empty tree".to_string()),
3120+
GroveError::CorruptedData("some unrelated corruption".to_string()),
3121+
GroveError::PathNotFound("no such subtree".to_string()),
3122+
] {
3123+
let label = format!("{other:?}");
3124+
assert!(
3125+
!is_ranked_chain_mismatch(&DriveError::GroveDB(Box::new(other))),
3126+
"must not be retried: {label}"
3127+
);
3128+
}
3129+
assert!(
3130+
!is_ranked_chain_mismatch(&DriveError::Drive(
3131+
drive::error::drive::DriveError::CorruptedDriveState("chain mismatch".to_string())
3132+
)),
3133+
"the marker alone must not make a non-grovedb error retriable"
3134+
);
3135+
}
30923136
}

packages/rs-drive/src/query/drive_document_ranked_query/tests.rs

Lines changed: 68 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ use super::index_picker::find_ranked_index_for_axis;
3535
use super::mode_detection::{detect_ranked_mode, detect_ranked_mode_v0};
3636
use super::*;
3737
use crate::drive::Drive;
38+
use crate::error::drive::DriveError;
3839
use crate::error::query::QuerySyntaxError;
3940
use crate::error::Error;
4041
use crate::query::having::{
@@ -57,6 +58,7 @@ use dpp::prelude::DataContract;
5758
use dpp::tests::json_document::json_document_to_contract;
5859
use dpp::version::PlatformVersion;
5960
use grovedb::element::indexed::compute_avg_fixed_point;
61+
use grovedb::operations::proof::indexed_axis::AxisEntries;
6062
use grovedb_costs::{CostContext, OperationCost};
6163
use std::collections::BTreeMap;
6264

@@ -1685,11 +1687,17 @@ fn a_deep_offset_does_not_cost_a_walk_of_the_skipped_region() {
16851687
.expect("the proof must build")
16861688
.len()
16871689
};
1690+
// Its own allowance, derived from what an envelope actually carries per
1691+
// level of the counted descent — a handful of collapsed ops, each a key
1692+
// plus a 32-byte hash and a count. Deliberately not `byte_slack`, which
1693+
// bounds *storage reads*: the two are unrelated quantities, and sharing
1694+
// one constant would let a change to either silently move the other.
1695+
let proof_size_slack = 128 * log2_population as usize;
16881696
let (shallow, mid) = (proof_bytes_at(0), proof_bytes_at(population / 2));
16891697
assert!(
1690-
shallow.abs_diff(mid) <= byte_slack as usize,
1698+
shallow.abs_diff(mid) <= proof_size_slack,
16911699
"the envelope's size must not grow with the offset it attests: {shallow} bytes at \
1692-
offset 0 against {mid} at offset {}",
1700+
offset 0 against {mid} at offset {}, allowing {proof_size_slack}",
16931701
population / 2
16941702
);
16951703
let (at_end, absurd) = (proof_bytes_at(population), proof_bytes_at(4_000_000_000));
@@ -1702,6 +1710,64 @@ fn a_deep_offset_does_not_cost_a_walk_of_the_skipped_region() {
17021710
);
17031711
}
17041712

1713+
/// **The shared decoder's two defensive arms, exercised directly.**
1714+
///
1715+
/// [`DriveDocumentRankedQuery::ranked_entries_from`] guards against an
1716+
/// envelope whose entries do not match the request: a different axis
1717+
/// shape, or more entries than `k` authorized. Neither is reachable
1718+
/// through grovedb today — the verifier builds the `AxisEntries` variant
1719+
/// from the caller's own `expected_axis`, and it enforces `k` itself — so
1720+
/// the arms exist for the day a decoder change decouples the two, and are
1721+
/// worth pinning precisely *because* nothing else can reach them. Calling
1722+
/// the decoder directly is the only way to prove they fire rather than
1723+
/// silently pass a mis-typed number to the caller.
1724+
#[test]
1725+
fn the_shared_entry_decoder_rejects_a_mismatched_axis_and_an_over_long_page() {
1726+
let (drive, contract) = setup_restaurants();
1727+
insert_docs(&drive, &contract, "tip", "amount", 1, &[("alpha", 10)]);
1728+
1729+
// A Count request handed Sum entries: the tag check upstream would
1730+
// normally have caught this, so reaching the caller would mean a
1731+
// `Sum` scalar being read as a document count.
1732+
let count_query = client_side_query(&contract, &RankedCase::count(false, Some(4)));
1733+
let error = count_query
1734+
.ranked_entries_from(AxisEntries::Sum(vec![(7, b"alpha".to_vec())]))
1735+
.expect_err("a Count query must not accept Sum entries");
1736+
assert!(
1737+
matches!(error, Error::Drive(DriveError::CorruptedDriveState(ref m))
1738+
if m.contains("different axis shape")),
1739+
"expected an axis-shape rejection, got {error}"
1740+
);
1741+
1742+
// More entries than `k`: the request authorized one page, and a
1743+
// longer list would mean the envelope committed a longer walk.
1744+
let one_entry_query = client_side_query(&contract, &RankedCase::sum(false, Some(1)));
1745+
let error = one_entry_query
1746+
.ranked_entries_from(AxisEntries::Sum(vec![
1747+
(10, b"alpha".to_vec()),
1748+
(9, b"beta".to_vec()),
1749+
]))
1750+
.expect_err("k = 1 must not accept two entries");
1751+
assert!(
1752+
matches!(error, Error::Drive(DriveError::CorruptedDriveState(ref m))
1753+
if m.contains("for k = 1")),
1754+
"expected a k rejection naming the limit, got {error}"
1755+
);
1756+
1757+
// And the happy path still decodes, so the guards are not simply
1758+
// rejecting everything.
1759+
let entries = one_entry_query
1760+
.ranked_entries_from(AxisEntries::Sum(vec![(10, b"alpha".to_vec())]))
1761+
.expect("a well-shaped single-entry page decodes");
1762+
assert_eq!(
1763+
entries,
1764+
vec![RankedEntry {
1765+
key: b"alpha".to_vec(),
1766+
value: RankedEntryValue::Sum(10),
1767+
}]
1768+
);
1769+
}
1770+
17051771
/// A proof of one page must not verify as another page of the same
17061772
/// ranking. `offset` is echoed in the envelope and re-checked, which is
17071773
/// what stops a server from answering "the 5th best" with a proof of

0 commit comments

Comments
 (0)