Fix Script::dust_value()'s calculation for non-P2*PKH script_pubkeys #579
Conversation
src/blockdata/script.rs
Outdated
| 3 * // The default dust relay fee is 3000 satoshi/kB (ie 1 sat/vByte) | ||
| if self.is_op_return() { | ||
| 0 | ||
| } else if self.is_witness_program() { | ||
| Self::WITNESS_OUTPUT_DUST_THRESHOLD | ||
| 32 + 4 + 1 + (107 / 4) + 4 + // The spend cost copied from Core | ||
| 8 + // The serialized size of the TxOut's amount field | ||
| self.consensus_encode(&mut ::std::io::sink()).unwrap() as u64 // The serialized size of this script_pubkey | ||
| } else { | ||
| Self::LEGACY_OUTPUT_DUST_THRESHOLD | ||
| 32 + 4 + 1 + 107 + 4 + // The spend cost copied from Core | ||
| 8 + // The serialized size of the TxOut's amount field | ||
| self.consensus_encode(&mut ::std::io::sink()).unwrap() as u64 // The serialized size of this script_pubkey |
There was a problem hiding this comment.
I will suggest improving readability of this fragment
if self.is_op_return() {
return 0
}
let script_pubkey_len = self.consensus_encode(&mut ::std::io::sink()).expect("I/O sink never fails") as u64;
let witness_reducer = (self.is_witness_program() as u8) * 4;
DEFAULT_DUST_RELAY_FEE * // The default dust relay fee is 3000 satoshi/kB (ie 1 sat/vByte)
(32 + 4 + 1 + 107 / witness_reducer + 4 // The spend cost copied from Core
+ 8 // The serialized size of the TxOut's amount field
+ script_pubkey_len)And introduce DEFAULT_DUST_RELAY_FEE const additionally to other values introduced by #584
There was a problem hiding this comment.
I'm not sure that DUST_RELAY_FEE is a useful constant - it is only used when calculating the dust threshold, not in policy in any other context. I'm not sure what a user would do with such a constant. As for your proposed change, note that witness_reducer can be 0 causing a divide-by-0 :).
The current code was written so as to make it clear its directly comparable to Core.
src/blockdata/script.rs
Outdated
| /// broadcastable on today's bitcoin network. | ||
| pub const WITNESS_OUTPUT_DUST_THRESHOLD: u64 = 294; | ||
| pub const P2WPKH_OUTPUT_DUST_THRESHOLD: u64 = 294; |
There was a problem hiding this comment.
Are these constants actually still all that useful now? If we fell for it someone else might do the same and just use one of the constants, not paying attention to the script type 😬
There was a problem hiding this comment.
Probably not, admittedly, I'm happy to remove them?
There was a problem hiding this comment.
Do if you want, I don't have too strong feelings either way. It just seemed somewhat redundant and I wanted to bring up the question.
There was a problem hiding this comment.
I do, in fact. Pushed.
The dust calculations added were only valid for P2WPKH and P2PKH outputs, and somehow this fact was missed in review, despite the upstream Core code being linked to and looked at by two reviewers and the author (me). Someday I will grow eyeballs, but that day is not today.
TheBlueMatt
dismissed stale reviews from sgeisler, dr-orlovsky, and apoelstra
via
15981c9
It doesn't really make sense to have a constant for every common script type's dust limit, instead we should just use the `Script::dust_value()` function to have users calculate it.
|
Ok, sorry about the churn, this should be good now, assuming we agree to drop the useless constants. |
|
If we drop the constants this is a breaking change. |
|
Right, didn't think of that 😬 so maybe only deprecate now and remove with 0.27.0? |
|
I think released 0.26.0 hadn't those constants yet, so removing them shouldn't be breaking |
|
Indeed, none is this has appeared in a release yet, so I figured it was fine to change. |
|
Ah! My bad |
The dust calculations added were only valid for P2WPKH and P2PKH
outputs, and somehow this fact was missed in review, despite the
upstream Core code being linked to and looked at by two reviewers
and the author (me).
Someday I will grow eyeballs, but that day is not today.