Skip to content

[consensus/marshal] Expand Digest Fetching#3722

Closed
patrick-ogrady wants to merge 29 commits into
mainfrom
expand-digest-fetching
Closed

[consensus/marshal] Expand Digest Fetching#3722
patrick-ogrady wants to merge 29 commits into
mainfrom
expand-digest-fetching

Conversation

@patrick-ogrady
Copy link
Copy Markdown
Contributor

Related: #3718

@cloudflare-workers-and-pages
Copy link
Copy Markdown

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

Deploying with  Cloudflare Workers  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 1a93809 May 15 2026, 07:06 AM

Comment thread consensus/src/marshal/core/actor.rs Outdated
}

/// Enqueue a resolver fetch for a block commitment.
async fn enqueue_block_fetch(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

remove this thin wrapper

.fetch(ResolverKey::request(Request::Notarized { round }))
.await;
} else if let (Some(height), BlockSubscriptionKey::Commitment(commitment)) = (height, key) {
if height <= self.last_processed_height {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

May need this to verify block (read parent)?

Comment thread consensus/src/marshal/core/cache.rs Outdated
metadata: Metadata<R, u8, (Epoch, Epoch)>,

/// Blocks fetched by digest during ancestry verification, indexed by height.
digest_blocks: prunable::Archive<TwoCap, R, <V::Block as Digestible>::Digest, V::StoredBlock>,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

put in caches

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

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.528 ms 1.532 ms +0.24% 10.00% ✅ PASS
qmdb::merkleize/variant=current::ordered::fixed::mmb chunk=256 keys=10000 ch=true sync=false 2.856 ms 2.830 ms -0.91% 10.00% ✅ PASS

Baseline commit(s): ced07a033227

Comment thread consensus/src/marshal/core/mailbox.rs Outdated
subscription.await.ok().map(V::into_inner)
}

async fn fetch_parent(self, block: Self::Block) -> Option<Self::Block> {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

make it clearer what actually fetches when subscribing.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

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

Deploying monorepo with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1a93809
Status: ✅  Deploy successful!
Preview URL: https://752626d6.monorepo-eu0.pages.dev
Branch Preview URL: https://expand-digest-fetching.monorepo-eu0.pages.dev

View logs

# Conflicts:
#	consensus/src/marshal/coding/marshaled.rs
#	consensus/src/marshal/core/cache.rs
#	consensus/src/marshal/standard/deferred.rs
#	consensus/src/marshal/standard/inline.rs
let block_rx = self.marshal.subscribe_by_digest(Some(round), digest).await;
let block_rx = self
.marshal
.subscribe_by_commitment(digest, CommitmentRequest::FetchByRound { round })
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the one big downside of this approach is that we can fetch the same object twice (won't dedup because of hint)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we refactor resolver to allow a multiple "retain keys" to a single "fetch key", we could minimize duplicate work.

@@ -510,7 +515,10 @@ where
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For data on the pending tip, we actually want to specify not to fetch the block (and just wait for it).

This would make it possible for us to starve the resolver capacity (because we won't prune that pending request until finalization).

Comment thread consensus/src/marshal/core/cache.rs Outdated
Finalization<S, V::Commitment>,
>,
/// Verified blocks stored by height.
verified_blocks_by_height:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we should be able to get rid of the verified by view (if we can track digest by height we don't need that)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

maybe call this "certified_blocks_by_height"?

/// Verified blocks stored by height.
verified_blocks_by_height:
prunable::Archive<TwoCap, R, <V::Block as Digestible>::Digest, V::StoredBlock>,
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We should ensure we don't fetch beyond the finalized floor here? if so, we should at least be sure we dedup requests?

verified_blocks_by_height:
prunable::Archive<TwoCap, R, <V::Block as Digestible>::Digest, V::StoredBlock>,
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure we ever need to fetch by notarized anymore?

@codecov
Copy link
Copy Markdown

codecov Bot commented May 8, 2026

Codecov Report

❌ Patch coverage is 91.40518% with 126 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.79%. Comparing base (6725867) to head (ab54361).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
consensus/src/marshal/core/actor.rs 88.42% 13 Missing and 9 partials ⚠️
consensus/src/marshal/standard/mod.rs 94.50% 19 Missing and 1 partial ⚠️
resolver/src/lib.rs 61.36% 17 Missing ⚠️
consensus/src/marshal/resolver/handler.rs 89.72% 15 Missing ⚠️
consensus/src/simplex/actors/resolver/state.rs 62.50% 12 Missing ⚠️
consensus/src/marshal/core/cache.rs 89.36% 7 Missing and 3 partials ⚠️
consensus/src/marshal/mocks/verifying.rs 69.69% 9 Missing and 1 partial ⚠️
consensus/src/marshal/coding/marshaled.rs 80.95% 7 Missing and 1 partial ⚠️
consensus/src/marshal/core/mailbox.rs 82.05% 7 Missing ⚠️
consensus/src/marshal/standard/validation.rs 76.92% 2 Missing and 1 partial ⚠️
... and 1 more
@@            Coverage Diff             @@
##             main    #3722      +/-   ##
==========================================
- Coverage   95.82%   95.79%   -0.04%     
==========================================
  Files         466      467       +1     
  Lines      185569   187086    +1517     
  Branches     4427     4467      +40     
==========================================
+ Hits       177825   179214    +1389     
- Misses       6324     6430     +106     
- Partials     1420     1442      +22     
Files with missing lines Coverage Δ
consensus/src/lib.rs 0.00% <ø> (ø)
consensus/src/marshal/ancestry.rs 95.00% <100.00%> (+0.35%) ⬆️
consensus/src/marshal/coding/mod.rs 98.66% <100.00%> (+0.10%) ⬆️
consensus/src/marshal/mocks/harness.rs 98.75% <100.00%> (-0.01%) ⬇️
consensus/src/marshal/mod.rs 54.54% <ø> (ø)
consensus/src/marshal/resolver/p2p.rs 100.00% <100.00%> (ø)
consensus/src/marshal/standard/deferred.rs 93.73% <100.00%> (+0.36%) ⬆️
consensus/src/marshal/standard/inline.rs 88.85% <100.00%> (+0.93%) ⬆️
consensus/src/simplex/actors/resolver/ingress.rs 100.00% <100.00%> (ø)
resolver/src/p2p/inflight.rs 100.00% <100.00%> (ø)
... and 14 more

... and 14 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 6725867...ab54361. 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.

# Conflicts:
#	consensus/src/marshal/coding/marshaled.rs
#	consensus/src/marshal/coding/mod.rs
#	consensus/src/marshal/core/actor.rs
#	consensus/src/marshal/core/mailbox.rs
#	consensus/src/marshal/mocks/harness.rs
#	consensus/src/marshal/resolver/handler.rs
#	consensus/src/marshal/resolver/p2p.rs
#	consensus/src/marshal/standard/deferred.rs
#	consensus/src/marshal/standard/inline.rs
#	consensus/src/marshal/standard/mod.rs
#	consensus/src/marshal/standard/validation.rs
#	consensus/src/simplex/actors/resolver/ingress.rs
#	consensus/src/simplex/actors/resolver/state.rs
#	examples/reshare/src/engine.rs
#	resolver/src/lib.rs
#	resolver/src/p2p/engine.rs
#	resolver/src/p2p/inflight.rs
#	resolver/src/p2p/ingress.rs
#	resolver/src/p2p/mocks/consumer.rs
#	resolver/src/p2p/mod.rs
@commonware-llm commonware-llm marked this pull request as ready for review May 15, 2026 06:41
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d0cd9d737f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +1124 to +1136
if height > self.last_processed_height {
if let Some(bounds) = self.epocher.containing(height) {
self.cache
.put_certified_block_by_height(
bounds.epoch(),
height,
digest,
block.clone().into(),
)
.await;
}
self.notify_subscribers(&block);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Deliver fetched ancestry blocks to waiting subscribers

When a Request::Block response is valid but the Ancestry subscriber metadata does not match (for example, a proposal carries an incorrect child height so expected_height != block.height()), this branch skips notify_subscribers entirely. The fetch_parent caller is waiting on subscribe_by_commitment, so dropping the context without notifying leaves that verification/certification task hanging until external cancellation instead of promptly receiving the parent and rejecting the block. This is a liveness regression on malformed proposals and can stall verification work.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d0cd9d7. Configure here.

}
self.notify_subscribers(&block);
}
false
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ancestry block delivery skips subscriber notification below floor

Medium Severity

In handle_deliver for Request::Block, the ancestry path only calls notify_subscribers when height > self.last_processed_height. If last_processed_height advances between the time a subscriber is registered (in handle_subscribe, where height > last_processed_height was true) and the time the resolver delivers the block, the block is silently dropped without notifying the still-registered block subscription waiter. The retain call in update_processed_height may cancel the resolver fetch, but if the response was already in-flight, the delivery arrives with valid subscribers while notify_subscribers is skipped. This can leave an AncestorStream's subscribe_parent call waiting indefinitely on a oneshot::Receiver that is never fulfilled nor closed.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d0cd9d7. Configure here.

@patrick-ogrady
Copy link
Copy Markdown
Contributor Author

Replaced by #3796

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant