Skip to content
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.

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

Modify BundleType to exclude the anchor & allow no bundle to be produced. #409

Merged
merged 2 commits into from
Dec 21, 2023

Conversation

nuttycom
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (78f5986) 80.98% compared to head (1892573) 80.99%.
Report is 6 commits behind head on main.

Files Patch % Lines
src/builder.rs 86.95% 3 Missing ⚠️
src/bundle.rs 0.00% 1 Missing ⚠️
src/tree.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #409      +/-   ##
==========================================
+ Coverage   80.98%   80.99%   +0.01%     
==========================================
  Files          31       31              
  Lines        3139     3146       +7     
==========================================
+ Hits         2542     2548       +6     
- Misses        597      598       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…duced.

This adds a flag to `BundleType` that, when set, requires a dummy-only
bundle to be produced even if no spends or outputs are added to the
builder, and when unset results in standard padding.
@nuttycom nuttycom force-pushed the required_bundles branch 3 times, most recently from 3523c70 to 3845686 Compare December 21, 2023 02:35
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

utACK 1892573 with nits.

Comment on lines +63 to +65
/// This anchor does not correspond to any valid anchor for a spend, so it
/// may only be used for coinbase bundles or in circumstances where Orchard
/// functionality is not active.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is entirely valid to use the empty tree in regular bundles. Doing so is equivalent to setting enable_spends = false - it proves publicly that the Orchard bundle spends no notes (but may still have real outputs). I think this comment is fine however because callers should reach for enable_spends in this case, rather than the empty anchor.

let mut builder = Builder::new(BundleType::Transactional(Flags::SPENDS_DISABLED, anchor));
let mut builder = Builder::new(
BundleType::Transactional {
flags: Flags::SPENDS_DISABLED,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this publicly announces that spends are disabled, which in a real shielding bundle may be highly undesirable. This doesn't matter in a test, but we should avoid doing this in examples that downstream crate users are likely to look at for reference.

@nuttycom nuttycom merged commit ba70c32 into main Dec 21, 2023
27 checks passed
@nuttycom nuttycom deleted the required_bundles branch December 21, 2023 15:16
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.

3 participants