[storage/qmdb/current] have current proof types implement codec by roberto-bayardo · Pull Request #3717 · commonwarexyz/monorepo

roberto-bayardo · 2026-05-05T18:11:10Z

Resolves: #3705

cloudflare-workers-and-pages · 2026-05-05T18:11:38Z

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Updated (UTC)
✅ Deployment successful! View logs	commonware-mcp	`19f1c7d`	May 05 2026, 11:14 PM

github-actions · 2026-05-05T18:13:10Z

Benchmark results

Tip

✅ PASSED: No benchmark exceeded the regression threshold.

Benchmark comparison table

Benchmark	Baseline (main)	Current	Delta	Threshold	Status
`qmdb::merkleize/variant=any::unordered::fixed::mmr keys=10000 ch=false sync=false`	1.731 ms	1.562 ms	-9.80%	10.00%	✅ PASS
`qmdb::merkleize/variant=current::ordered::fixed::mmb chunk=256 keys=10000 ch=true sync=false`	2.973 ms	2.895 ms	-2.65%	10.00%	✅ PASS

Baseline commit(s): dec68db8757a

Copilot

Pull request overview

Adds commonware-codec serialization support to QMDB “current” proof structures so they can be shipped over the wire and round-tripped in tests.

Changes:

Implement Write, EncodeSize, and Read for RangeProof<F, D> with bounded decoding via ReadRangeExt.
Implement Write, EncodeSize, and Read for OperationProof<F, D, N> (threading config to the embedded RangeProof).
Add unit tests that round-trip encode/decode for both proof types.

cloudflare-workers-and-pages · 2026-05-05T18:26:14Z

Deploying monorepo with Cloudflare Pages

Latest commit:	`19f1c7d`
Status:	✅ Deploy successful!
Preview URL:	https://22c6cefa.monorepo-eu0.pages.dev
Branch Preview URL:	https://codec-proofs.monorepo-eu0.pages.dev

View logs

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

patrick-ogrady

we should better leverage codec

patrick-ogrady · 2026-05-05T21:37:13Z

+            Self::KeyValue(op_proof, update) => {
+                EXCLUSION_TAG_KEY_VALUE.write(buf);
+                op_proof.write(buf);
+                update.key.write(buf);


Is there a reason we don’t define write on update?

AI slop, fixed.

patrick-ogrady · 2026-05-05T21:37:13Z

+            Self::Commit(op_proof, value) => {
+                EXCLUSION_TAG_COMMIT.write(buf);
+                op_proof.write(buf);
+                value.is_some().write(buf);


Option write is already supported by Codec!

patrick-ogrady · 2026-05-05T21:37:13Z

+            }
+            EXCLUSION_TAG_COMMIT => {
+                let op_proof = OperationProof::<F, D, N>::read_cfg(buf, max_digests)?;
+                let value = if bool::read(buf)? {


see comment about option support

patrick-ogrady · 2026-05-05T21:37:13Z

+        }
+        self.unfolded_prefix_peaks.write(buf);
+        self.partial_chunk_digest.is_some().write(buf);
+        if let Some(digest) = &self.partial_chunk_digest {


see comment about option support

patrick-ogrady · 2026-05-05T21:47:40Z

Another finding:

path: storage/src/qmdb/current/proof.rs
line: 748
severity: medium
---
`OperationProof` is now decodable from untrusted bytes, but `verify` indexes the decoded bitmap chunk before enforcing the chunk-size invariant that `current::init` enforces for real DBs. A decoded `OperationProof<_, _, 0>` reaches `verify` and panics in `BitMap::<0>::get_bit_from_chunk` instead of returning `false`. Please reject `N == 0` before bitmap indexing and before the delegated `RangeProof::verify` path can divide by `CHUNK_SIZE_BITS`.

Test command: `just test -p commonware-storage test_decoded_operation_proof_zero_sized_chunk_does_not_panic`

Observed result: the test fails with a panic at `utils/src/bitmap/mod.rs:535`: `attempt to calculate the remainder with a divisor of zero`.

<details><summary>Regression test patch</summary>

```patch
diff --git a/storage/src/qmdb/current/proof.rs b/storage/src/qmdb/current/proof.rs
index d49ea1d80..ce5ac8b73 100644
--- a/storage/src/qmdb/current/proof.rs
+++ b/storage/src/qmdb/current/proof.rs
@@ -913,6 +913,40 @@ mod tests {
         assert_eq!(decoded, proof);
     }
 
+    #[test]
+    fn test_decoded_operation_proof_zero_sized_chunk_does_not_panic() {
+        type F = mmb::Family;
+        const N: usize = 0;
+        const MAX_DIGESTS: usize = 64;
+
+        let proof = OperationProof::<F, sha256::Digest, N> {
+            loc: mmb::Location::new(0),
+            chunk: [],
+            range_proof: RangeProof {
+                proof: Proof::<F, sha256::Digest> {
+                    leaves: mmb::Location::new(0),
+                    inactive_peaks: 0,
+                    digests: vec![],
+                },
+                pre_prefix_acc: None,
+                unfolded_prefix_peaks: vec![],
+                partial_chunk_digest: None,
+                ops_root: Sha256::hash(b"ops"),
+            },
+        };
+        let decoded =
+            OperationProof::<F, sha256::Digest, N>::decode_cfg(proof.encode(), &MAX_DIGESTS)
+                .unwrap();
+        let root = Sha256::hash(b"root");
+
+        let result = std::panic::catch_unwind(|| {
+            let mut hasher = Sha256::new();
+            decoded.verify(&mut hasher, 7u8, &root)
+        });
+        assert!(result.is_ok(), "verify panicked for zero-sized bitmap chunks");
+        assert!(!result.unwrap(), "zero-sized bitmap chunk proof verified");
+    }
+
     #[test_traced]
     fn test_range_proof_verifies_for_mmb_multi_peak_chunk() {
         let executor = deterministic::Runner::default();

One unique actionable finding remained after deduplication: decoded OperationProof values with zero-sized bitmap chunks can panic during verification instead of failing cleanly.

patrick-ogrady · 2026-05-05T21:48:45Z

Another finding:

path: storage/src/qmdb/current/proof.rs
line: 748
severity: medium
---
`OperationProof` is now decodable from untrusted bytes, but `verify` indexes the decoded bitmap chunk before enforcing the chunk-size invariant that `current::init` enforces for real DBs. A decoded `OperationProof<_, _, 0>` reaches `verify` and panics in `BitMap::<0>::get_bit_from_chunk` instead of returning `false`. Please reject `N == 0` before bitmap indexing and before the delegated `RangeProof::verify` path can divide by `CHUNK_SIZE_BITS`.

Test command: `just test -p commonware-storage test_decoded_operation_proof_zero_sized_chunk_does_not_panic`

Observed result: the test fails with a panic at `utils/src/bitmap/mod.rs:535`: `attempt to calculate the remainder with a divisor of zero`.

<details><summary>Regression test patch</summary>

```patch
diff --git a/storage/src/qmdb/current/proof.rs b/storage/src/qmdb/current/proof.rs
index d49ea1d80..ce5ac8b73 100644
--- a/storage/src/qmdb/current/proof.rs
+++ b/storage/src/qmdb/current/proof.rs
@@ -913,6 +913,40 @@ mod tests {
         assert_eq!(decoded, proof);
     }
 
+    #[test]
+    fn test_decoded_operation_proof_zero_sized_chunk_does_not_panic() {
+        type F = mmb::Family;
+        const N: usize = 0;
+        const MAX_DIGESTS: usize = 64;
+
+        let proof = OperationProof::<F, sha256::Digest, N> {
+            loc: mmb::Location::new(0),
+            chunk: [],
+            range_proof: RangeProof {
+                proof: Proof::<F, sha256::Digest> {
+                    leaves: mmb::Location::new(0),
+                    inactive_peaks: 0,
+                    digests: vec![],
+                },
+                pre_prefix_acc: None,
+                unfolded_prefix_peaks: vec![],
+                partial_chunk_digest: None,
+                ops_root: Sha256::hash(b"ops"),
+            },
+        };
+        let decoded =
+            OperationProof::<F, sha256::Digest, N>::decode_cfg(proof.encode(), &MAX_DIGESTS)
+                .unwrap();
+        let root = Sha256::hash(b"root");
+
+        let result = std::panic::catch_unwind(|| {
+            let mut hasher = Sha256::new();
+            decoded.verify(&mut hasher, 7u8, &root)
+        });
+        assert!(result.is_ok(), "verify panicked for zero-sized bitmap chunks");
+        assert!(!result.unwrap(), "zero-sized bitmap chunk proof verified");
+    }
+
     #[test_traced]
     fn test_range_proof_verifies_for_mmb_multi_peak_chunk() {
         let executor = deterministic::Runner::default();

One unique actionable finding remained after deduplication: decoded OperationProof values with zero-sized bitmap chunks can panic during verification instead of failing cleanly.

Ah, this is dumb. N is controlled.

patrick-ogrady · 2026-05-05T21:50:23Z

 }
+
+#[cfg(test)]
+mod codec_tests {


I think this can just be part of tests?

patrick-ogrady · 2026-05-05T21:50:52Z

+                let value = Option::<V::Value>::read_cfg(buf, value_cfg)?;
+                Ok(Self::Commit(op_proof, value))
+            }
+            _ => Err(commonware_codec::Error::Invalid(


I think there is an "InvalidEnum"

patrick-ogrady · 2026-05-05T22:20:34Z

We should add conformance tests for these new proof types.

patrick-ogrady · 2026-05-05T22:28:26Z

max_digests is currently enforced three times, not once: Proof::read_cfg caps proof.digests, then RangeProof::read_cfg separately allows up to max_digests more entries in both unfolded_prefix_peaks and unfolded_suffix_peaks. Because OperationProof::read_cfg, KeyValueProof::read_cfg, and ExclusionProof::read_cfg all forward the same cfg unchanged, every new decoder added in this PR can accept a well-formed payload whose total digest allocation is roughly 3 * max_digests. That defeats the caller's allocation budget and reintroduces the DoS vector this cfg is supposed to prevent. Please enforce a single remaining digest budget across all three vectors, or change the cfg shape to explicit per-field budgets and document that contract.

roberto-bayardo · 2026-05-05T22:31:58Z

max_digests is currently enforced three times, not once: Proof::read_cfg caps proof.digests, then RangeProof::read_cfg separately allows up to max_digests more entries in both unfolded_prefix_peaks and unfolded_suffix_peaks. Because OperationProof::read_cfg, KeyValueProof::read_cfg, and ExclusionProof::read_cfg all forward the same cfg unchanged, every new decoder added in this PR can accept a well-formed payload whose total digest allocation is roughly 3 * max_digests. That defeats the caller's allocation budget and reintroduces the DoS vector this cfg is supposed to prevent. Please enforce a single remaining digest budget across all three vectors, or change the cfg shape to explicit per-field budgets and document that contract.

This is fine since max-digests is generally around 64 -- hard to DoS with that few digests even if it is *N. I can document better.

roberto-bayardo · 2026-05-05T22:38:45Z

We should add conformance tests for these new proof types.

done

patrick-ogrady · 2026-05-05T22:38:48Z

This is fine since max-digests is generally around 64 -- hard to DoS with that few digests even if it is *N. I can document better.

What do we do in other proof decode functions?

roberto-bayardo · 2026-05-05T22:39:33Z

This is fine since max-digests is generally around 64 -- hard to DoS with that few digests even if it is *N. I can document better.

What do we do in other proof decode functions?

The other proof decoder only had one field of digests.

patrick-ogrady · 2026-05-05T22:39:41Z

patrick-ogrady · 2026-05-05T23:21:14Z

Should fully unblock exoware!

codecov · 2026-05-06T00:15:07Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.83%. Comparing base (dec68db) to head (19f1c7d).
⚠️ Report is 1 commits behind head on main.

@@           Coverage Diff            @@
##             main    #3717    +/-   ##
========================================
  Coverage   95.83%   95.83%            
========================================
  Files         458      458            
  Lines      180650   180964   +314     
  Branches     4218     4218            
========================================
+ Hits       173118   173430   +312     
- Misses       6186     6187     +1     
- Partials     1346     1347     +1

Files with missing lines	Coverage Δ
storage/src/qmdb/current/ordered/db.rs	`87.50% <100.00%> (+1.93%)`	⬆️
storage/src/qmdb/current/ordered/mod.rs	`99.85% <100.00%> (+0.03%)`	⬆️
storage/src/qmdb/current/proof.rs	`96.47% <100.00%> (+0.48%)`	⬆️

... and 3 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dec68db...19f1c7d. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

roberto-bayardo requested a review from Copilot May 5, 2026 18:11

github-actions Bot assigned roberto-bayardo May 5, 2026

Copilot started reviewing on behalf of roberto-bayardo May 5, 2026 18:12 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

Comment thread storage/src/qmdb/current/proof.rs Outdated

roberto-bayardo added this to Tracker May 5, 2026

roberto-bayardo moved this to In Progress in Tracker May 5, 2026

roberto-bayardo marked this pull request as ready for review May 5, 2026 19:47

roberto-bayardo requested review from Copilot and patrick-ogrady May 5, 2026 19:47

Copilot started reviewing on behalf of roberto-bayardo May 5, 2026 19:48 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

patrick-ogrady requested changes May 5, 2026

View reviewed changes

patrick-ogrady reviewed May 5, 2026

View reviewed changes

roberto-bayardo force-pushed the codec-proofs branch from 8abd15a to c752eb4 Compare May 5, 2026 21:54

roberto-bayardo requested a review from patrick-ogrady May 5, 2026 21:59

roberto-bayardo added 6 commits May 5, 2026 15:06

have current proof types implement codec

984b299

more proof types

de8d540

fix ai slop

c4f440b

tweaks

3392d62

merge test modules

d253175

lint

145ed9f

roberto-bayardo force-pushed the codec-proofs branch from ab849d8 to 638cb18 Compare May 5, 2026 22:08

rebase

e42644b

roberto-bayardo force-pushed the codec-proofs branch from 638cb18 to e42644b Compare May 5, 2026 22:08

conformance + doc tweaks

ab8e6c8

patrick-ogrady reviewed May 5, 2026

View reviewed changes

Comment thread storage/conformance.toml

patrick-ogrady May 5, 2026

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

❤️

christopherwxyz reacted with heart emoji

enforce max-digest budget instead of per-field

31a9821

patrick-ogrady reviewed May 5, 2026

View reviewed changes

Comment thread storage/src/qmdb/current/proof.rs

removed unnecessary checked_sub

19f1c7d

roberto-bayardo force-pushed the codec-proofs branch from 469937c to 19f1c7d Compare May 5, 2026 23:14

roberto-bayardo requested a review from patrick-ogrady May 5, 2026 23:18

patrick-ogrady approved these changes May 5, 2026

View reviewed changes

patrick-ogrady merged commit b55a48c into main May 5, 2026
174 of 175 checks passed

patrick-ogrady deleted the codec-proofs branch May 5, 2026 23:29

github-project-automation Bot moved this from In Progress to Done in Tracker May 5, 2026

Uh oh!

Conversation

roberto-bayardo commented May 5, 2026

Uh oh!

cloudflare-workers-and-pages Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Workers

Uh oh!

github-actions Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

cloudflare-workers-and-pages Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying monorepo with Cloudflare Pages

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

patrick-ogrady left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrick-ogrady commented May 5, 2026

Uh oh!

patrick-ogrady commented May 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrick-ogrady commented May 5, 2026

Uh oh!

patrick-ogrady commented May 5, 2026

Uh oh!

roberto-bayardo commented May 5, 2026

Uh oh!

roberto-bayardo commented May 5, 2026

Uh oh!

patrick-ogrady commented May 5, 2026

Uh oh!

roberto-bayardo commented May 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patrick-ogrady commented May 5, 2026

Uh oh!

Uh oh!

codecov Bot commented May 6, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

cloudflare-workers-and-pages Bot commented May 5, 2026 •

edited

Loading

github-actions Bot commented May 5, 2026 •

edited

Loading

cloudflare-workers-and-pages Bot commented May 5, 2026 •

edited

Loading