-
Notifications
You must be signed in to change notification settings - Fork 72
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 n30239 notes and questions #778
base: master
Are you sure you want to change the base?
Conversation
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.
Good stuff already, thanks @tdb3! I've not yet reviewed 30239, so my comments lack context and please ignore them if you think they don't make sense.
Since it's a pretty big PR, would be good to expand on the Notes to give readers some more context on the questions they'll have to answer later (without spoonfeeding it). As you mentioned offline too, at least a few code questions would be nice, but definitely starting with a good concept focus is 🔥
1. What is _ephemeral_ dust? | ||
|
||
1. What are the specific proposed rules for ephemeral dust? |
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.
These questions seem like they largely overlap, so perhaps they should be merged into one?
|
||
1. Which types of tests were created for the PR? What is the purpose of each type of test? | ||
|
||
1. Why is the test in [this commit](https://github.com/bitcoin/bitcoin/pull/30239/commits/fd22f9e2f4709a00bd31b739a7edadf98a728831) important? |
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 think the answer to this question is pretty much copy-pasting the commit message, so perhaps it'd be good to approach this from a different angle? Perhaps something along the lines of the following could be a more adventurous alternative: "what happens when we broadcast a transaction with properties <...> and a node with -dustrelayfee=0
? How do we protect against this / what makes you confident this won't be an issue \ ..."
|
||
1. What could happen if restrictions on the the child tx were not in place? | ||
|
||
1. Which types of tests were created for the PR? What is the purpose of each type of test? |
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 think perhaps this question can be dropped. This PR already has plenty of concept to discuss, so I think the discussion on different kinds of tests is best kept for other PRs.
|
||
1. Why is the test in [this commit](https://github.com/bitcoin/bitcoin/pull/30239/commits/fd22f9e2f4709a00bd31b739a7edadf98a728831) important? | ||
|
||
1. Could a miner choose to mine transactions containing dust? Is this a concern? |
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.
Perhaps add "what could you do to address the concerns, if any?"
|
||
- Recommend reading from earlier review clubs: | ||
- [#30352: Add PayToAnchor(P2A), OP 1 <0x4e73> as standard output script for spending](https://bitcoincore.reviews/30352) |
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.
In similar style to what was done for n30352, would be good to highlight how this PR relates to n30352, how it's different, ... To give some context to people jumping in.
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.
Also, in this case, I'd put the link to n30352 at the top of the notes, and make it required reading
|
||
1. What are the specific proposed rules for ephemeral dust? | ||
|
||
1. Why is it important to impose a fee restriction? |
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.
To make it more open-ended, could add follow-ups like:
"Which specific vulnerabilities can you imagine without the fee restriction? Can you think of (in your view) positive use cases that are not possible because of the fee restriction?"
Draft notes and questions added. Suggestions welcome.