-
Notifications
You must be signed in to change notification settings - Fork 250
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
Cleanups, refactoring, and placeholder for TEX addresses #1423
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1423 +/- ##
==========================================
+ Coverage 63.03% 63.07% +0.04%
==========================================
Files 127 127
Lines 14784 14821 +37
==========================================
+ Hits 9319 9349 +30
- Misses 5465 5472 +7 ☔ View full report in Codecov by Sentry. |
b66e93e
to
761e9ae
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.
I don't think that OutPoint::NULL
needs to be a public constant (and it may not need to be a constant at all. Otherwise LGTM.
761e9ae
to
f005ab9
Compare
9e63bb0
to
830eb7e
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 830eb7e.
components/zcash_encoding/src/lib.rs
Outdated
/// Returns the serialized size of a vector of `u8` as written by `[Vector::write]`. | ||
pub fn serialized_size(vec: &[u8]) -> usize { | ||
let length = vec.len(); | ||
CompactSize::serialized_size(length) + length | ||
} |
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 don't like using Vector::serialized_size
(an untyped name) for specifically &[u8]
. Instead we should either have some kind of Measurable
trait, or this should take two arguments (which can be s.len(), 1
) for s: &[u8]
):
/// Returns the serialized size of a vector of `u8` as written by `[Vector::write]`. | |
pub fn serialized_size(vec: &[u8]) -> usize { | |
let length = vec.len(); | |
CompactSize::serialized_size(length) + length | |
} | |
/// Returns the serialized size of a vector. | |
/// | |
/// - `element_count` is the number of elements in the vector. | |
/// - `element_size` is the size in bytes of an element encoding as written by the | |
/// function provided to `[Vector::write]`. | |
pub fn serialized_size(element_count: usize, element_size: usize) -> usize { | |
CompactSize::serialized_size(element_count) + element_count * element_size | |
} |
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 arithmetic can overflow that way. As I had it, it can't for counts that ever occur in practice (due to MAX_COMPACT_SIZE
). Will renaming the method to serialized_size_of_u8_vec
do?
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 addition in the existing PR diff can also overflow (in fewer circumstances, but still) for very large length
; I don't consider that a reason to not have the generic code. If we want to ensure it doesn't, then do a checked mul and panic on overflow (and document that the method panics if element_count * element_size + 9
exceeds usize::MAX
.
But if you don't want to do that, then renaming to Vector::serialized_size_for_u8
would be fine.
@@ -322,6 +322,11 @@ impl Script { | |||
Vector::write(&mut writer, &self.0, |w, e| w.write_u8(*e)) | |||
} | |||
|
|||
/// Returns the length of this script as encoded (including the initial CompactSize). | |||
pub fn serialized_size(&self) -> usize { |
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.
Note to us for after this PR: we need to bump the zcash_encoding
dependency of zcash_primitives
to 0.2.1
(once that is released) due to this new method (which uses a method not present in zcash_encoding 0.2.0
).
&Vec::<Infallible>::new()[..], | ||
), | ||
&orchard_fees::EmptyBundleView, | ||
&DustOutputPolicy::new(DustAction::AllowDustChange, Some(NonNegativeAmount::ZERO)), |
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 was the change_with_transparent_payments
test altered to use this dust output policy (instead of DustOutputPolicy::default()
)? AFAICT the test is not transparent-only (it has a Sapling input), and therefore the change to the fee code should not have affected it. Is this change perhaps necessary due to the bug I identified? If so, revert it.
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.
Please move this comment to #1430.
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.
Moved.
result, | ||
Ok(balance) if | ||
balance.proposed_change() == [ChangeValue::sapling(NonNegativeAmount::const_from_u64(1000), None)] && | ||
balance.fee_required() == NonNegativeAmount::const_from_u64(10000) |
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.
AFAICT this test passes despite the bug I found, for two reasons:
- The proposed change output counts are ignored at change output creation time; the single change output is always created even when
single_change_output_policy
says no change outputs should be created. - The
grace_actions
value for ZIP 317 means that the fee calculation allows for the extra logical action that this change output consumes.
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.
Please move this comment to #1430 if it is still relevant.
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.
Shouldn't apply now that the bug has been fixed, assuming tests pass.
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 830eb7e
result, | ||
Ok(balance) if | ||
balance.proposed_change() == [ChangeValue::sapling(NonNegativeAmount::const_from_u64(1000), None)] && | ||
balance.fee_required() == NonNegativeAmount::const_from_u64(10000) |
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.
Bug: the Sapling builder always pads outputs, so if there is one Sapling output, the builder will always create two, and so this transaction will require a 15,000 ZAT fee: 5000 ZAT for the transparent action, and 10,000 ZAT for the two Sapling actions created by padding. This is why I don't like the padding being part of the low-level builder; everything potentially downstream of it needs to predict that behavior.
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.
Please move this comment to #1430 if it is still relevant.
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 issue with dust change and builder padding is blocking.
21cb651
to
783833e
Compare
8d4ce74
to
4e42357
Compare
.dust_threshold() | ||
.unwrap_or(default_dust_threshold); | ||
|
||
if proposed_change < dust_threshold { |
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.
If proposed_change
is zero (i.e. total_in == total_out_plus_fee_with_change
), there is no actual dust output being proposed. I think that while AllowDustChange
and AddDustToFee
make sense, the behavior of DustAction::Reject
is somewhat nonintuitive in this case, in particular because we want to produce zero-valued change outputs in order to hide the exact input value in the case that a transaction has exactly two real outputs.
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.
Please move this comment to #1430 if it is still relevant.
} | ||
DustAction::AllowDustChange => simple_case(), | ||
DustAction::AddDustToFee => { | ||
let fee_with_dust = (total_in - total_out).expect("can't happen"); |
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.
let fee_with_dust = (total_in - total_out).expect("can't happen"); | |
let fee_with_dust = (total_in - total_out).unwrap(); |
or else provide a better expectation message than "can't happen"
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.
Please move this comment to #1430.
.ok_or_else(overflow)?; | ||
if fee_with_dust > reasonable_fee { | ||
// Defend against losing money by using AddDustToFee with a too-high dust threshold. | ||
simple_case() |
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 case doesn't make sense to me. If fee_with_dust
is that large, won't that result in a change output earlier? Shouldn't this return an error?
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.
Please move this comment to #1430.
4e42357
to
34fbbc8
Compare
Signed-off-by: Daira-Emma Hopwood <[email protected]>
changing its type from `ShieldedProtocol` to `PoolType`. Also fix compilation errors when the "orchard" feature is used without the "transparent-inputs" feature. Signed-off-by: Daira-Emma Hopwood <[email protected]>
if a memo is given for the transparent pool. Use `ChangeValue::shielded` to avoid this error case when creating a `ChangeValue` known to be for a shielded pool. Co-authored-by: Jack Grigg <[email protected]> Signed-off-by: Daira-Emma Hopwood <[email protected]>
`PoolType::{Shielded(_), Transparent}`. Signed-off-by: Daira-Emma Hopwood <[email protected]>
Signed-off-by: Daira-Emma Hopwood <[email protected]>
Signed-off-by: Daira-Emma Hopwood <[email protected]>
Signed-off-by: Daira-Emma Hopwood <[email protected]>
inputs and outputs. Co-authored-by: Kris Nuttycombe <[email protected]> Signed-off-by: Daira-Emma Hopwood <[email protected]>
`zcash_primitives::legacy::Script::serialized_size`. Use the latter in `zcash_primitives::transaction::fees::transparent::OutputView::serialized_size`. Signed-off-by: Daira-Emma Hopwood <[email protected]>
Signed-off-by: Daira-Emma Hopwood <[email protected]>
Signed-off-by: Daira-Emma Hopwood <[email protected]>
665c06f
to
98216be
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 98216be
Signed-off-by: Daira-Emma Hopwood <[email protected]>
Signed-off-by: Daira-Emma Hopwood <[email protected]>
Signed-off-by: Daira-Emma Hopwood <[email protected]>
Signed-off-by: Daira-Emma Hopwood <[email protected]>
Signed-off-by: Daira-Emma Hopwood <[email protected]>
98216be
to
0b7f60d
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 0b7f60d
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.
Self-ACK
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 0b7f60d
This PR includes several other cleanups. Changes related to change strategy will be split out to another PR.
fixes #1424