milksad
/
rust-bitcoin-unsafe-fast
14
0
Fork 0
Code Issues Pull Requests Activity

Merge rust-bitcoin/rust-bitcoin#699: Huffman Bug Fix 

f2a6827982 Fix BinaryHeap direction for Taproot Huffman Encoder (Jeremy Rubin)
cccd75d004 Fix Weighting Addition to never error on overflow + prevent overflows from ever happening with wider integers (Jeremy Rubin)

Pull request description:

  I noticed one cleanup & one bugfix while looking into the huffman algorithm:

  1) the cleanup: we can use a u128 to guarantee no overflows, and saturating_add to guarantee reasonable behavior in any case
  2) the bug: the binary heap is a max heap so the behavior ends up merging the nodes of the most likely entries repeatedly. a huffman encoder requires merging the least likely elements, so it should be reversed.

ACKs for top commit:
  sanket1729:
    ACK f2a6827982
  dr-orlovsky:
    utACK f2a6827982

Tree-SHA512: 07cadb8dd5cc2b7e6ae3ebc2c1639de054e41bcd7f3b7d338a93e77fd200c9591a89915aaae5d9f5313eff3d94032fdfe06d89fda1e2398881b711d149e9afe9
This commit is contained in:
Dr Maxim Orlovsky 2021-11-23 19:22:33 +01:00
parent d614b6c759 f2a6827982
commit 5286d0ab0c
No known key found for this signature in database
GPG Key ID: FFC0250947E5C6F7
1 changed files with 7 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
src/util/taproot.rs
View File

@ -20,6 +20,7 @@ use io;
use secp256k1::{self, Secp256k1}; use secp256k1::{self, Secp256k1};
use core::fmt; use core::fmt;
use core::cmp::Reverse;
#[cfg(feature = "std")] #[cfg(feature = "std")]
use std::error; use std::error;
@ -210,9 +211,9 @@ impl TaprootSpendInfo {
I: IntoIterator<Item = (u64, Script)>, I: IntoIterator<Item = (u64, Script)>,
C: secp256k1::Verification, C: secp256k1::Verification,
{ {
let mut node_weights = BinaryHeap::<(u64, NodeInfo)>::new(); let mut node_weights = BinaryHeap::<(Reverse<u128>, NodeInfo)>::new();
for (p, leaf) in script_weights { for (p, leaf) in script_weights {
node_weights.push((p, NodeInfo::new_leaf_with_ver(leaf, LeafVersion::default()))); node_weights.push((Reverse(p as u128), NodeInfo::new_leaf_with_ver(leaf, LeafVersion::default())));
} }
if node_weights.is_empty() { if node_weights.is_empty() {
return Err(TaprootBuilderError::IncompleteTree); return Err(TaprootBuilderError::IncompleteTree);
@ -222,7 +223,10 @@ impl TaprootSpendInfo {
let (p1, s1) = node_weights.pop().expect("len must be at least two"); 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"); 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 // Insert the sum of first two in the tree as a new node
let p = p1.checked_add(p2).ok_or(TaprootBuilderError::ScriptWeightOverflow)?; // 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));
node_weights.push((p, NodeInfo::combine(s1, s2)?)); node_weights.push((p, NodeInfo::combine(s1, s2)?));
} }
// Every iteration of the loop reduces the node_weights.len() by exactly 1 // Every iteration of the loop reduces the node_weights.len() by exactly 1
@ -803,8 +807,6 @@ pub enum TaprootBuilderError {
IncompleteTree, IncompleteTree,
/// Called finalize on a empty tree /// Called finalize on a empty tree
EmptyTree, EmptyTree,
/// Script weight overflow
ScriptWeightOverflow,
} }
impl fmt::Display for TaprootBuilderError { impl fmt::Display for TaprootBuilderError {
@ -832,9 +834,6 @@ impl fmt::Display for TaprootBuilderError {
TaprootBuilderError::EmptyTree => { TaprootBuilderError::EmptyTree => {
write!(f, "Called finalize on an empty tree") write!(f, "Called finalize on an empty tree")
} }
TaprootBuilderError::ScriptWeightOverflow => {
write!(f, "Script weight overflow in Huffman tree construction")
},
} }
} }
} }