-
Notifications
You must be signed in to change notification settings - Fork 8
feat: enable random anti-fee sniping #5
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
base: master
Are you sure you want to change the base?
Conversation
6a39ba9 to
94237e8
Compare
|
Thanks @aagbotemi I'm still in the process of reviewing. I agree it would be nice to resolve the clippy error - my other suggestion would be to globally allow the lints in |
Thank you @ValuedMammal. Clippy error and large enum variant error in the CI build has been fixed. For subsequent ones(if there is), I'll open another for 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.
I have some review notes
- For imports, prefer
allocoverstd. This way we can actually support no-std. - I want to see a test of some kind that creates a PSBT with
enable_anti_fee_snipingand checks the expected values of the locktime and/or sequence.
Noted. |
42fe314 to
cfe7b5b
Compare
cfe7b5b to
9041b2f
Compare
|
We should also be able to remove the clippy allow attributes in |
Alright, I will remove the clippy allow attributes in |
|
I'm still in favor of using |
Fixed! I've updated the code to use |
5506018 to
9fde5c1
Compare
ff19f53 to
c4dd696
Compare
|
In general I think the git history would be cleaner if we didn't merge with merge commits but instead rebase the commits in this PR. |
cfa9826 to
4649e8d
Compare
Alright, I've done the cleanup |
|
Approach 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.
Approach ACK 4649e8d.
I guess this will be squashed before merging, but I would reserve the revert type in commit messages for those actually reverting other commits, not manual commits undoing something.
I also checked the references in the BIP and bitcoin core seems to have implemented something similar back in the time.
| fn random_range(rng: &mut OsRng, end: u32) -> u32 { | ||
| let max = u32::MAX; | ||
| let max_multiple = max - (max % end); | ||
|
|
||
| loop { | ||
| let n = rng.next_u32(); | ||
| if n < max_multiple { | ||
| return n % end; | ||
| } | ||
| } | ||
| } |
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.
Here you are defining the largest contiguous unbiased region within the numeric range representable by the u32 type. From what I have discovered, (-end) % end is the preferred version in the latest literature, although the performance is almost the same. Probably the compiler optimizes both forms to the same instructions, but I would use (-end) % (end) to fingerprint the technique you're using here for future code readers.
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 tested end.wrapping_neg() % end for/same as (-end) % end but it causes infinite loops.
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 has now been fixed using Debiased Modulo (Twice) — OpenBSD's Method
src/utils.rs
Outdated
| random_range(rng, probability) == 0 | ||
| } | ||
|
|
||
| // Return a random value in the range [0, end]. |
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.
You are trying to add a docstring here or just a comment? Use /// for docstrings.
Also, the range is not inclusive: [0, end), or [0, end - 1]
src/selection.rs
Outdated
| } | ||
| } | ||
|
|
||
| assert!(used_locktime || used_sequence); |
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.
Not needed anymore if using the loop.
src/selection.rs
Outdated
| used_sequence = true; | ||
| // One of the inputs should have modified sequence | ||
| let has_modified_sequence = tx.input.iter().any(|txin| { | ||
| dbg!(&txin.sequence.to_consensus_u32()); |
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 would remove this.
src/selection.rs
Outdated
| let mut used_locktime = false; | ||
| let mut used_sequence = false; | ||
|
|
||
| for _ in 0..50 { |
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 would loop until I have one case for both and then I would exit. 50 seems to much.
src/selection.rs
Outdated
| let mut used_locktime = false; | ||
| let mut used_sequence = false; | ||
|
|
||
| for _ in 0..100 { |
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.
Same than below, a while checking both conditions are met is better.
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 add to the above comment, you could count the number of loops and then check in a final assert than the amount of loops is less than a sane number, i.e.: assert!(loops < 20)
src/selection.rs
Outdated
|
|
||
| pub fn setup_test_input(confirmation_height: u32) -> anyhow::Result<(Input, absolute::Height)> { | ||
| let secp = Secp256k1::new(); | ||
| let s = "tr([83737d5e/86h/1h/0h]tpubDDR5GgtoxS8fJyjjvdahN4VzV5DV6jtbcyvVXhEKq2XtpxjxBXmxH3r8QrNbQqHg4bJM1EGkxi7Pjfkgnui9jQWqS7kxHvX6rhUeriLDKxz/0/*)"; |
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 prefer to have the full name of the thing. Also, could this strings be constants somewhere?
examples/anti_fee_sniping.rs
Outdated
| // Locktime is used, if rbf is disabled or any input requires locktime | ||
| // (e.g. non-taproot, unconfirmed, or >65535 confirmation) or there are | ||
| // no taproot inputs or the 50/50 coin flip chose locktime (USE_NLOCKTIME_PROBABILITY) | ||
| // Further-back randomness with 10% chance (FURTHER_BACK_PROBABILITY), | ||
| // will subtract a random 0–99 block offset to desynchronize from tip |
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 this is more understandable if the conditions are enumerated as bullet points.
src/utils.rs
Outdated
| rbf_enabled: bool, | ||
| ) -> Result<(), CreatePsbtError> { | ||
| const MAX_RELATIVE_HEIGHT: u32 = 65_535; | ||
| const USE_NLOCKTIME_PROBABILITY: u32 = 2; |
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 name is misleading, the intended probability is 50%. Maybe 50_PERCENT_PROBABILITY_RANGE is more accurate, if there is no one better.
src/utils.rs
Outdated
| const MAX_RELATIVE_HEIGHT: u32 = 65_535; | ||
| const USE_NLOCKTIME_PROBABILITY: u32 = 2; | ||
| const MIN_SEQUENCE_VALUE: u32 = 1; | ||
| const FURTHER_BACK_PROBABILITY: u32 = 10; |
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.
Same here, the intended probability is 10%.
Thank you for the review. I'm working on fixing them. |
4649e8d to
63ecd47
Compare
63ecd47 to
5a0b8cb
Compare
5a0b8cb to
782c3e3
Compare
This PR implements Anti-Fee-Sniping with randomization as discussed in issue #4
Notes to the reviewers
The implementation adds randomization to anti-fee-sniping behavior:
cargo fmtandcargo clippybefore committingCloses #4