Add Script:dust_value() to get minimum output value for a spk #566
Conversation
TheBlueMatt
force-pushed
the
2021-02-dust-value
branch
from
cba9c5b
to
9b567a0
Compare
TheBlueMatt
force-pushed
the
2021-02-dust-value
branch
from
9b567a0
to
ad0d82d
Compare
TheBlueMatt
force-pushed
the
2021-02-dust-value
branch
from
ad0d82d
to
6622de4
Compare
| /// Gets the minimum value an output with this script should have in order to be | ||
| /// broadcastable on today's bitcoin network. | ||
| pub fn dust_value(&self) -> u64 { | ||
| if self.is_op_return() { |
There was a problem hiding this comment.
When looking at core it also captures the case where a script is too big making the UTXO unspendable. But that would only make this implementation more conservative which might be ok since it's such a fringe use case and not consensus critical.
There was a problem hiding this comment.
Hmm, right - are those scripts broadcastable at all or are they nonstandard?
There was a problem hiding this comment.
Those scripts are not possible by consensus rules all scripts(bare, p2sh, witness_v0). But post tapscript, those scripts will be broadcastable.
https://github.com/bitcoin/bips/blob/master/bip-0342.mediawiki#resource-limits
We can change the isWitnessProgram check to check only v0 programs, but this can also be done in taproot upgrade #503. But this could have easily been one of the things we forgot to add.
There was a problem hiding this comment.
@sgeisler Nevermind, I was wrong. This function is about scriptPubkey and not about the witness Program inside it. The witness program can be more 10,000 in Tapscript, but scriptpubkey is always 35.
There was a problem hiding this comment.
Right, I'm not even talking about if its valid (its obviously not possible to spend), but is it standard - can you broadcast a transaction with such a script in an output and have it be mined, even if the output is unspendable.
|
utACK 6622de4 |
|
I don't think anything bad will come from erring on the safe side (especially with such an exotic corner case) in case of non-consensus rules, so I'll go ahead and merge. Feel free to open a follow-up issue/PR. |
|
I wish that this PR, which is based on a 6-month-old version of I spot checked the merge commit with a bunch of feature flags and it seems good. |
|
Oops, sorry about that. I forgot to check what it was based in before creating. Can we make CI rebase before testing?
… On Feb 10, 2021, at 12:53, Andrew Poelstra ***@***.***> wrote:
I wish that this PR, which is based on a 6-month-old version of master (which is a full major version behind and therefore cannot be tested with the current set of feature flags) and which nobody tested, had not been merged like this.
I spot checked the merge commit with a bunch of feature flags and it seems good.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Sorry, the PR seemed innocent enough and merged it 😦 putting "based off recent master" on my checklist. I guess I'll ping you more often in the future @apoelstra. |
Can't you just checkout |
|
@sgeisler yes, but my PR-testing script does not like to test things that are already descendends of It usually doesn't matter whether something is based off of that recent a |
|
Hmm, it would be nice to have CI enforce something concrete to catch it earlier. It’s not to hard to rebase during CI, but we could also add a time limit or version limit to enforce the age of the common ancestor.
… On Feb 10, 2021, at 17:39, Andrew Poelstra ***@***.***> wrote:
@sgeisler yes, but my PR-testing script does not like to test things that are already descendends of master. It is also important to me that the individual commits compile ... which is not the case for this PR (without changing a bunch of cargo flags) even though it is a one-commit PR and even though the merge commit does compile.
It usually doesn't matter whether something is based off of that recent a master ... although I always do take a look and consider interactions that may be caused by having too many parallel branches being merged in ... but in this case we were a full six months behind.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
No description provided.