Skip to content

fix(drive): consolidate historical contract proof verification retry logic #3165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thepastaclaw wants to merge 5 commits into
base: v3.1-dev
Choose a base branch
from
+220 −87
Open

fix(drive): consolidate historical contract proof verification retry logic #3165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8197175
fix(drive): consolidate historical contract proof verification retry …
thepastaclaw Feb 28, 2026
63e16fe
fix(drive): retry contract verification only on errors
thepastaclaw Feb 28, 2026
1c6f1a2
test(drive): require contract existence for explicit history flags
thepastaclaw Feb 28, 2026
73c264e
test(drive): make keeps-history verification assertions explicit
thepastaclaw Feb 28, 2026
1ab6d20
fix(drive): restore Ok(None) retry and strengthen test assertions
thepastaclaw Feb 28, 2026
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
90 changes: 47 additions & 43 deletions packages/rs-drive/src/verify/contract/verify_contract/v0/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,49 @@ impl Drive {
contract_id: [u8; 32],
platform_version: &PlatformVersion,
) -> Result<(RootHash, Option<DataContract>), Error> {
let path_query = match (
let keeps_history = contract_known_keeps_history.unwrap_or(false);

let result = Self::verify_contract_v0_given_history(
proof,
keeps_history,
is_proof_subset,
in_multiple_contract_proof_form,
contract_known_keeps_history.unwrap_or_default(),
) {
contract_id,
platform_version,
);

if contract_known_keeps_history.is_none() {
match &result {
Ok((_, Some(_))) => result,
_ => {
tracing::debug!(
?contract_id,
"retrying contract verification with history enabled"
);
Self::verify_contract_v0_given_history(
proof,
true,
is_proof_subset,
in_multiple_contract_proof_form,
contract_id,
platform_version,
)
Copy link
Copy Markdown
Contributor

@lklimek lklimek Mar 6, 2026

Choose a reason for hiding this comment

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

SEC-001 (LOW) — Restore diagnostic context in retry log

The old code logged the original result before retrying:

tracing::debug!(?path_query, error=?e, "retrying contract verification with history enabled");

The new code only logs contract_id, losing the original result and path query context that helps diagnose verification issues.

Suggestion: log the original result at debug level before retrying:

tracing::debug!(?contract_id, ?result, "retrying contract verification with history enabled");

🤖 Claudius

}
}
} else {
result
}
}

fn verify_contract_v0_given_history(
proof: &[u8],
keeps_history: bool,
is_proof_subset: bool,
in_multiple_contract_proof_form: bool,
contract_id: [u8; 32],
platform_version: &PlatformVersion,
) -> Result<(RootHash, Option<DataContract>), Error> {
let path_query = match (in_multiple_contract_proof_form, keeps_history) {
(true, true) => Self::fetch_historical_contracts_query(&[contract_id]),
(true, false) => Self::fetch_non_historical_contracts_query(&[contract_id]),
(false, true) => Self::fetch_contract_with_history_latest_query(contract_id, true),
Expand All @@ -67,25 +106,7 @@ impl Drive {
&platform_version.drive.grove_version,
)
};
let (root_hash, mut proved_key_values) = match result.map_err(Error::from) {
Ok(ok_result) => ok_result,
Err(e) => {
return if contract_known_keeps_history.is_none() {
tracing::debug!(?path_query,error=?e, "retrying contract verification with history enabled");
// most likely we are trying to prove a historical contract
Self::verify_contract(
proof,
Some(true),
is_proof_subset,
in_multiple_contract_proof_form,
contract_id,
platform_version,
)
} else {
Err(e)
};
}
};
let (root_hash, mut proved_key_values) = result.map_err(Error::from)?;
if proved_key_values.is_empty() {
return Err(Error::Proof(ProofError::WrongElementCount {
expected: 1,
Expand All @@ -94,7 +115,7 @@ impl Drive {
}
if proved_key_values.len() == 1 {
let (path, key, maybe_element) = proved_key_values.remove(0);
if contract_known_keeps_history.unwrap_or_default() {
if keeps_history {
if path != contract_keeping_history_root_path(&contract_id) {
return Err(Error::Proof(ProofError::CorruptedProof(
"we did not get back an element for the correct path for the historical contract".to_string(),
Expand Down Expand Up @@ -125,26 +146,9 @@ impl Drive {
.map_err(Error::from)
})
})
.transpose();
match contract {
Ok(contract) => Ok((root_hash, contract)),
Err(e) => {
if contract_known_keeps_history.is_some() {
// just return error
Err(e)
} else {
tracing::debug!(?path_query,error=?e, "retry contract verification with history enabled");
Self::verify_contract(
proof,
Some(true),
is_proof_subset,
in_multiple_contract_proof_form,
contract_id,
platform_version,
)
}
}
}
.transpose()?;

Ok((root_hash, contract))
} else {
Err(Error::Proof(ProofError::TooManyElements(
"expected one contract id",
Expand Down
90 changes: 47 additions & 43 deletions packages/rs-drive/src/verify/contract/verify_contract_return_serialization/v0/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,49 @@ impl Drive {
contract_id: [u8; 32],
platform_version: &PlatformVersion,
) -> Result<VerifyContractReturn, Error> {
let path_query = match (
let keeps_history = contract_known_keeps_history.unwrap_or(false);

let result = Self::verify_contract_return_serialization_v0_given_history(
proof,
keeps_history,
is_proof_subset,
in_multiple_contract_proof_form,
contract_known_keeps_history.unwrap_or_default(),
) {
contract_id,
platform_version,
);

if contract_known_keeps_history.is_none() {
match &result {
Ok((_, Some(_))) => result,
_ => {
tracing::debug!(
?contract_id,
"retrying contract verification with history enabled"
);
Self::verify_contract_return_serialization_v0_given_history(
proof,
true,
is_proof_subset,
in_multiple_contract_proof_form,
contract_id,
platform_version,
)
}
}
} else {
result
}
}

fn verify_contract_return_serialization_v0_given_history(
proof: &[u8],
keeps_history: bool,
is_proof_subset: bool,
in_multiple_contract_proof_form: bool,
contract_id: [u8; 32],
platform_version: &PlatformVersion,
) -> Result<VerifyContractReturn, Error> {
let path_query = match (in_multiple_contract_proof_form, keeps_history) {
(true, true) => Self::fetch_historical_contracts_query(&[contract_id]),
(true, false) => Self::fetch_non_historical_contracts_query(&[contract_id]),
(false, true) => Self::fetch_contract_with_history_latest_query(contract_id, true),
Expand All @@ -69,25 +108,7 @@ impl Drive {
&platform_version.drive.grove_version,
)
};
let (root_hash, mut proved_key_values) = match result.map_err(Error::from) {
Ok(ok_result) => ok_result,
Err(e) => {
return if contract_known_keeps_history.is_none() {
tracing::debug!(?path_query,error=?e, "retrying contract verification with history enabled");
// most likely we are trying to prove a historical contract
Self::verify_contract_return_serialization_v0(
proof,
Some(true),
is_proof_subset,
in_multiple_contract_proof_form,
contract_id,
platform_version,
)
} else {
Err(e)
};
}
};
let (root_hash, mut proved_key_values) = result.map_err(Error::from)?;
if proved_key_values.is_empty() {
return Err(Error::Proof(ProofError::WrongElementCount {
expected: 1,
Expand All @@ -96,7 +117,7 @@ impl Drive {
}
if proved_key_values.len() == 1 {
let (path, key, maybe_element) = proved_key_values.remove(0);
if contract_known_keeps_history.unwrap_or_default() {
if keeps_history {
if path != contract_keeping_history_root_path(&contract_id) {
return Err(Error::Proof(ProofError::CorruptedProof(
"we did not get back an element for the correct path for the historical contract".to_string(),
Expand Down Expand Up @@ -134,26 +155,9 @@ impl Drive {
))
})
})
.transpose();
match contract {
Ok(contract) => Ok((root_hash, contract)),
Err(e) => {
if contract_known_keeps_history.is_some() {
// just return error
Err(e)
} else {
tracing::debug!(?path_query,error=?e, "retry contract verification with history enabled");
Self::verify_contract_return_serialization_v0(
proof,
Some(true),
is_proof_subset,
in_multiple_contract_proof_form,
contract_id,
platform_version,
)
}
}
}
.transpose()?;

Ok((root_hash, contract))
} else {
Err(Error::Proof(ProofError::TooManyElements(
"expected one contract id",
Expand Down
127 changes: 126 additions & 1 deletion packages/rs-drive/tests/query_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ use base64::Engine;
#[cfg(feature = "server")]
use dpp::block::block_info::BlockInfo;
use dpp::data_contract::accessors::v0::DataContractV0Getters;
use dpp::data_contract::accessors::v0::DataContractV0Setters;
use dpp::data_contract::config::v0::DataContractConfigSettersV0;
use dpp::data_contract::config::v1::DataContractConfigSettersV1;
use dpp::data_contract::conversion::value::v0::DataContractValueConversionMethodsV0;
use dpp::data_contract::document_type::methods::DocumentTypeV0Methods;
Expand Down Expand Up @@ -918,6 +920,14 @@ pub fn setup_withdrawal_tests(
#[cfg(feature = "server")]
/// Sets up the References contract to test queries on.
pub fn setup_references_tests(_count: u32, _seed: u64) -> (Drive, DataContract) {
setup_references_tests_with_keeps_history(_count, _seed, false)
}

pub fn setup_references_tests_with_keeps_history(
_count: u32,
_seed: u64,
keeps_history: bool,
) -> (Drive, DataContract) {
let drive = setup_drive(Some(DriveConfig::default()));

let db_transaction = drive.grove.start_transaction();
Expand All @@ -939,7 +949,9 @@ pub fn setup_references_tests(_count: u32, _seed: u64) -> (Drive, DataContract)
"tests/supporting_files/contract/references/references_with_contract_history.json",
None,
None,
None::<fn(&mut DataContract)>,
Some(|contract: &mut DataContract| {
contract.config_mut().set_keeps_history(keeps_history);
}),
Some(&db_transaction),
None,
);
Expand Down Expand Up @@ -4687,6 +4699,119 @@ mod tests {
);
}

#[test]
fn test_contract_keeps_history_verify_with_unknown_history_flag() {
Copy link
Copy Markdown
Contributor

@lklimek lklimek Mar 6, 2026

Choose a reason for hiding this comment

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

PROJ-002 (LOW) — Test does not verify retry was actually exercised

Test 1 asserts the end result is correct but doesn't prove the retry path was taken. If a GroveDb change made non-historical queries accidentally succeed on historical proofs, this test would still pass while the retry logic becomes dead code.

Consider capturing the tracing debug log ("retrying contract verification with history enabled") via tracing-test subscriber and asserting it was emitted exactly once.

🤖 Claudius

// Regression test: when contract_known_keeps_history is None,
// verification must still succeed for historical contracts.
let (drive, contract) = setup_references_tests_with_keeps_history(10, 3334, true);
let platform_version = PlatformVersion::latest();

// Apply an update so the contract has an actual history entry and latest historical path.
let mut latest_contract = contract.clone();
latest_contract.set_version(contract.version() + 1);
drive
.apply_contract(
&latest_contract,
BlockInfo {
time_ms: 1,
..Default::default()
},
true,
StorageFlags::optional_default_as_cow(),
None,
platform_version,
)
.expect("contract update should be applied");

let root_hash = drive
.grove
.root_hash(None, &platform_version.drive.grove_version)
.unwrap()
.expect("there is always a root hash");

let contract_proof = drive
.prove_contract(latest_contract.id().into_buffer(), None, platform_version)
.expect("expected to get proof");

// Test 1: None (unknown) - verification must succeed, and may use retry behavior.
let (proof_root_hash, proof_contract) = Drive::verify_contract(
contract_proof.as_slice(),
None,
false,
Copy link
Copy Markdown
Contributor

@lklimek lklimek Mar 6, 2026

Choose a reason for hiding this comment

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

PROJ-003 (INFO) — No coverage for in_multiple_contract_proof_form=true

All four sub-cases use in_multiple_contract_proof_form=false. The verify_contracts (plural) function calls verify_contract with (None, true, true) — so this retry path is used in production but not directly tested by this PR or anywhere else in the test suite. Consider adding a test with true, or testing verify_contracts with a historical contract.

🤖 Claudius

false,
latest_contract.id().into_buffer(),
platform_version,
)
.expect("verification with None should succeed");
assert_eq!(root_hash, proof_root_hash);
assert_eq!(
latest_contract,
proof_contract.expect("contract exists and was updated — None retry must return it")
);

// Test 2: Some(true) - direct historical, must succeed since proof was generated
// for a historical contract
let (proof_root_hash_2, proof_contract_2) = Drive::verify_contract(
contract_proof.as_slice(),
Some(true),
false,
false,
latest_contract.id().into_buffer(),
platform_version,
)
.expect("verification with Some(true) should succeed for historical contract");
assert_eq!(root_hash, proof_root_hash_2);
assert_eq!(
latest_contract,
proof_contract_2.expect("expected contract with explicit history flag")
);

// Test 3: Some(false) - explicit non-historical contract must verify to existing value.
let (non_historical_drive, non_historical_contract) =
setup_references_tests_with_keeps_history(10, 3334, false);
let non_historical_root_hash = non_historical_drive
.grove
.root_hash(None, &platform_version.drive.grove_version)
.unwrap()
.expect("there is always a root hash");
let non_historical_proof = non_historical_drive
.prove_contract(
non_historical_contract.id().into_buffer(),
None,
platform_version,
)
.expect("expected to get proof");
let (proof_root_hash_3, proof_contract_3) = Drive::verify_contract(
non_historical_proof.as_slice(),
Some(false),
false,
false,
non_historical_contract.id().into_buffer(),
platform_version,
)
.expect("verification with Some(false) should return the existing contract");
assert_eq!(non_historical_root_hash, proof_root_hash_3);
assert_eq!(
non_historical_contract,
proof_contract_3.expect("expected contract with explicit non-history flag")
);

// Test 4: verify_contract_return_serialization with None
let (proof_root_hash_4, proof_contract_4) = Drive::verify_contract_return_serialization(
contract_proof.as_slice(),
None,
false,
false,
latest_contract.id().into_buffer(),
platform_version,
)
.expect("return_serialization with None should succeed for historical contract");
assert_eq!(root_hash, proof_root_hash_4);
let (deserialized, _bytes) =
proof_contract_4.expect("contract exists — return_serialization must return it");
assert_eq!(latest_contract, deserialized);
}

Copy link
Copy Markdown
Contributor

@lklimek lklimek Mar 6, 2026

Choose a reason for hiding this comment

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

PROJ-001 (LOW) — Add test for non-existent contract with None history flag

The test covers the happy path (historical contract exists, None triggers successful retry) but does not cover the opposite: a contract ID that genuinely does not exist with contract_known_keeps_history=None.

Empirical testing confirms this case is safe (absence proof verifies on both paths, retry returns Ok((_, None)) correctly), but an explicit test would prevent regressions if the retry logic changes:

#[test]
fn test_verify_non_existent_contract_with_unknown_history_flag() {
    let (drive, _contract) = setup_references_tests(10, 3334);
    let platform_version = PlatformVersion::latest();
    let fake_contract_id = [255u8; 32];
    let contract_proof = drive
        .prove_contract(fake_contract_id, None, platform_version)
        .expect("expected to get proof for non-existent contract");
    let (_, proof_returned_contract) = Drive::verify_contract(
        contract_proof.as_slice(),
        None,
        false,
        false,
        fake_contract_id,
        platform_version,
    )
    .expect("expected verification to succeed for non-existent contract");
    assert!(
        proof_returned_contract.is_none(),
        "expected None for a non-existent contract, got Some",
    );
}

No such test exists in the codebase.

🤖 Claudius

#[cfg(feature = "server")]
#[test]
fn test_dpns_query_first_version() {
Expand Down
Loading