Improve timelock code #414
Conversation
There was a problem hiding this comment.
ACK e870519. Thanks for looking into this. Apart from the refactor here, there is also great value in someone looking at this code again.
| // If more than one branch may be taken, and some other branch has a requirement | ||
| // that conflicts with this one, set `contains_combination` | ||
| if k >= 2 { | ||
| if k > 1 { |
There was a problem hiding this comment.
This one is an improvement and I will ACK this change. But generally speaking, the symbols > >= should represent the way to think about the code rather than the conversion.
If while explaining some concept, we usually say "Two or more" rather than "more than one", it makes sense to write >=2.
There was a problem hiding this comment.
Interesting, thanks for your thoughts. I tried to explain this if statement out loud to myself and I was unsure if I would say "if there are more than 1 ..." or "if there are 2 or more". I think I'd say "more than one" but I think that's just from reading > statements many times, so might be circular reasoning :) Amusingly I then opened the editor at this line and there is the following code comment
// If more than one branch may be taken, and some other branch has a requirement
// that conflicts with this one, set `contains_combination`.
|
This would now has conflicts because of changes in #340. Would need rebase |
Bitcoin Core uses the identifier `LOCKTIME_THRESHOLD` but we are using `HEIGHT_TIME_THRESHOLD`. Its not immediately clear that one is better than the other so lets use the same identifier as Bitcoin Core.
It is more typical to write `if k > 1` than `if k >= 2`.
Use more descriptive names for the 'combine' methods while removing the 'timelocks' bit of the method names.
We currently use the form "time lock" and also "timelock" (implies `TimeLock`, `Timelock`). Which we use is not important but we should be uniform. Favour "timelock" (and `Timelock`).
Add a simple unit test showing how `combine_threshold` works. Test asserts that multiple timelocks of the same type result in unspendable paths if threshold is 2.
When using `fold` the first argument is the accumulator, we can use the typical functional programming identifier `acc` with no loss of clarity. The other argument is a timelock, this is clear because we are iterating `timelocks`, use `t` instead of `sub_timelock` with no loss of clarity (subjective).
|
Force push is rebase only. |
b0840100b4e87f798caef35fb03ab6f3b914ed46 Use terse functional programming terms (Tobin C. Harding) 6f3303d5eba527d551c03dbcfe9234f69cf2d2f3 Improve docs on TimelockInfo (Tobin C. Harding) b2f6ef04a0d819a18332660c904d8802bfa20975 Add unit test for combine_threshold (Tobin C. Harding) a36f6085f0e96fbd74bab086c9cfa3d5ad35367e Be uniform in spelling of timelock (Tobin C. Harding) 51de643d3cdb33c389a62ab475fe3d6d39f65b9b Rename comibine methods (Tobin C. Harding) ef6803f946e285549157da161fce71217383c032 Use > 1 instead of >= 2 (Tobin C. Harding) d1fdbaaf3f5af43f1d201a3d0dfbc15f877b0888 Use LOCKTIME_THRESHOLD same as bitcoin core (Tobin C. Harding) Pull request description: Done while working on a [timelock module](rust-bitcoin/rust-bitcoin#994). This is all the initial patches (except one) from rust-bitcoin/rust-miniscript#408 (which is a PR displaying usage of the new timelock module). Note, does _not_ do the 'make `TimelockInfo` fields pub crate' change - I was unsure if this was correct. Top commit has no ACKs. Tree-SHA512: aa54e2d884f7cb1fb5dcb2d828ada29830ac4a7a09b04797e6e2fb448df476cbb31345841735e6cf4d5b7b1f6783781859778805fffef708f259fe780c6ba677
3 participants
Done while working on a timelock module. This is all the initial patches (except one) from #408 (which is a PR displaying usage of the new timelock module).
Note, does not do the 'make
TimelockInfofields pub crate' change - I was unsure if this was correct.