-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add a public bundle-builder function as an alternative to the mutable builder. #404
Conversation
The term `recipient` is commonly used to refer to the address to which a note is sent; however, a bundle may include multiple outputs to the same recipient. This change is intended to clarify this usage. Co-authored-by: Daira Emma Hopwood <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #404 +/- ##
==========================================
- Coverage 81.07% 80.99% -0.08%
==========================================
Files 31 31
Lines 3112 3141 +29
==========================================
+ Hits 2523 2544 +21
- Misses 589 597 +8 ☔ View full report in Codecov by Sentry. |
c6d49ed
to
2e2c161
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed as of 2e2c161.
/// Defined in [Zcash Protocol Spec § 4.8.3: Dummy Notes (Orchard)][orcharddummynotes]. | ||
/// | ||
/// [orcharddummynotes]: https://zips.z.cash/protocol/nu5.pdf#orcharddummynotes | ||
fn dummy(rng: &mut impl RngCore) -> Self { | ||
pub fn dummy(rng: &mut impl RngCore) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be public? SpendInfo::dummy
is not public (so there's an API asymmetry now), and the builder is responsible for applying padding internally.
pub fn dummy(rng: &mut impl RngCore) -> Self { | |
fn dummy(rng: &mut impl RngCore) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want it to be possible for callers to add dummy actions beyond the two required padding; they can already do so by manually sending zero value to a random address, so exposing this convenience method doesn't add any capability that they don't already have, and I don't see there as being a substantive maintenance burden.
c482781
to
422c5e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed as of 422c5e3.
let num_actions = bundle_type | ||
.num_actions(num_requested_spends, num_requested_outputs) | ||
.map_err(|_| BuildError::BundleTypeNotSatisfiable)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now a bit unintuitive, because it can potentially include both real spends and outputs, and dummies beyond what the bundle type's padding requires. This opens up the API to user-defined extra padding that can potentially make their transactions distinguishable from network transactions.
That being said, the way that BundleType::num_actions
works means that this doesn't prevent the BundleType
's padding rule from being applied, so this is a hazard that should be documented, not a bug per se.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The users can already add padding by just sending zeros to arbitrary addresses, so it doesn't change anything.
e85e415
to
74ba740
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 74ba740
Co-authored-by: str4d <[email protected]>
74ba740
to
938b203
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-utACK 938b203
An alternative/extension to #403