Add explicit control of padding to the Builder API. #403
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #403 +/- ##
==========================================
- Coverage 81.07% 81.01% -0.07%
==========================================
Files 31 31
Lines 3112 3123 +11
==========================================
+ Hits 2523 2530 +7
- Misses 589 593 +4 ☔ View full report in Codecov by Sentry. |
nuttycom
force-pushed
the
padding_rule
branch
3 times, most recently
from
ce1cb35 to
054e91e
Compare
nuttycom
force-pushed
the
padding_rule
branch
5 times, most recently
from
ef5b328 to
6c7648d
Compare
src/builder.rs
Outdated
| impl PaddingRule { | ||
| /// The default padding rule, which ensures that the constructed bundle will contain at least 2 | ||
| /// actions if any genuine Orchard spends or outputs are requested. | ||
| pub const DEFAULT: PaddingRule = PaddingRule::PadTo(MIN_ACTIONS); |
There was a problem hiding this comment.
Nit: the old behaviour is equivalent to PaddingRule::Require(MIN_ACTIONS) (which is why we could always return a bundle).
nuttycom
force-pushed
the
padding_rule
branch
2 times, most recently
from
0722bd4 to
89bc5cd
Compare
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]>
nuttycom
force-pushed
the
padding_rule
branch
4 times, most recently
from
d9945df to
b6bc396
Compare
benches/circuit.rs
Outdated
| @@ -7,7 +7,7 @@ use criterion::{BenchmarkId, Criterion}; | |||
| use pprof::criterion::{Output, PProfProfiler}; | |||
|
|
|||
| use orchard::{ | |||
| builder::Builder, | |||
| builder::{Builder, PaddingRule}, | |||
There was a problem hiding this comment.
Update these files for the refactor.
src/builder.rs
Outdated
| /// An enumeration of rules for construction of dummy actions that may be applied to Orchard bundle | ||
| /// construction. |
There was a problem hiding this comment.
| /// An enumeration of rules for construction of dummy actions that may be applied to Orchard bundle | |
| /// construction. | |
| /// An enumeration of rules for Orchard bundle construction. |
src/builder.rs
Outdated
| /// | ||
| /// The returned bundle will have no proof or signatures; these can be applied with | ||
| /// [`Bundle::create_proof`] and [`Bundle::apply_signatures`] respectively. | ||
| pub fn build<V: TryFrom<i64>>( | ||
| mut self, | ||
| mut rng: impl RngCore, | ||
| padding_rule: &BundleType, |
There was a problem hiding this comment.
| padding_rule: &BundleType, | |
| bundle_type: &BundleType, |
src/builder.rs
Outdated
| .unwrap(); | ||
| let num_real_spends = self.spends.len(); | ||
| let num_real_outputs = self.outputs.len(); | ||
| let num_actions = padding_rule |
There was a problem hiding this comment.
| let num_actions = padding_rule | |
| let num_actions = bundle_type |
| /// to receive funds. | ||
| #[derive(Debug)] | ||
| pub struct Builder { | ||
| spends: Vec<SpendInfo>, | ||
| recipients: Vec<RecipientInfo>, | ||
| outputs: Vec<OutputInfo>, | ||
| flags: Flags, |
There was a problem hiding this comment.
To some extent, the flags are related to the BundleType. In particular, if BundleType::Coinbase is used, flags.spends_enabled() must be true (this check is missing from Builder::build; the num_real_spends == 0 check is insufficient). So I'm not sure if this means we should be constructing to builder with BundleType, or ensuring we remember to add all necessary checks in Builder::build.
There was a problem hiding this comment.
The bundle type sort of implies the flags; what if BundleType::Transactional actually carried a flags payload, with the coinbase bundle type hardcoding the flags to (false, true)?
There was a problem hiding this comment.
And then we check the flags in build rather than in the add_* methods?
nuttycom
force-pushed
the
padding_rule
branch
from
b6bc396 to
0a257d6
Compare
No description provided.