Huffman Bug Fix #699
Merged
Huffman Bug Fix #699
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
a currently failing test #700 that passes if rebased on this branch. |
Kixunil
reviewed
Nov 15, 2021
| // N.B.: p1 + p2 can never actually saturate as you would need to have 2**64 max u64s | ||
| // from the input to overflow. However, saturating is a reasonable behavior here as | ||
| // huffman tree construction would treat all such elements as "very likely". | ||
| let p = Reverse(p1.0.saturating_add(p2.0)); |
There was a problem hiding this comment.
If it can never happen, why not just have +?
There was a problem hiding this comment.
i'd rather define the behavior correctly here should it ever overflow rather than wrapping around. Perhaaps defensively, but it'd be possible someone were to update the types/sizes later without appreciating this point.
|
I think u128 is supported on all platforms, either with a native type or a wrapper. We could also do the same trick with u32 and u64 and it wouldn't be an issue either (u32 is plenty IMO). |
sanket1729
reviewed
Nov 15, 2021
| @@ -220,7 +221,10 @@ impl TaprootSpendInfo { | |||
| let (p1, s1) = node_weights.pop().expect("len must be at least two"); | |||
| let (p2, s2) = node_weights.pop().expect("len must be at least two"); | |||
| // Insert the sum of first two in the tree as a new node | |||
| let p = p1.checked_add(p2).ok_or(TaprootBuilderError::ScriptWeightOverflow)?; | |||
There was a problem hiding this comment.
Can you also remove the Error variant from the error enum?
… from ever happening with wider integers
JeremyRubin
force-pushed
the
huffman-cleanups
branch
from
9a40597
to
f2a6827
Compare
dr-orlovsky
added a commit
that referenced
this pull request
Dec 11, 2021
1518517 Decrease Huffman weight type to 32 bits (Jeremy Rubin) Pull request description: This builds on #699 but is the more bikesheddable part since it changes the API. > u32 of weight should be enough for any branch. -- Bill Gates ACKs for top commit: dr-orlovsky: utACK 1518517 Kixunil: ACK 1518517 Tree-SHA512: 9c507ae6129dda8dc069b0a142181a78cf89cb3ebf9d2169c46662822cb4ea9ed075bf484528f5399fe0ed383a425174a702e2d685f31c246f5a86c46ed17c3a
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed one cleanup & one bugfix while looking into the huffman algorithm: