Skip to content

Decrease Huffman Weights to u32#701

Merged
dr-orlovsky merged 1 commit intorust-bitcoin:masterfrom
JeremyRubin:huffman-size
Dec 11, 2021
Merged

Decrease Huffman Weights to u32#701
dr-orlovsky merged 1 commit intorust-bitcoin:masterfrom
JeremyRubin:huffman-size

Conversation

@JeremyRubin
Copy link
Copy Markdown
Contributor

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

@JeremyRubin
Copy link
Copy Markdown
Contributor Author

rebased :)

Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 1518517

@Kixunil Kixunil added the one ack PRs that have one ACK, so one more can progress them label Dec 2, 2021
@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Dec 2, 2021

Looks legit but I'd like someone more experienced with probabilities than myself to confirm this is a good idea.

@JeremyRubin
Copy link
Copy Markdown
Contributor Author

@Kixunil basically this restricts our "fidelity" in expressing relative probabilities to maximally 4 billion to one, and introduces a possible "optimization glitch" when we have more than 4 billion branches all of maximal weight (well formedness still fine).

I think it's very unlikely that we need to express something as more than 4 billion times more likely very often -- i.e. if you think you might use a signing path once per second for lightning, you'd use the other path once every 126 years.

I don't think this patch is critical, but some folks may not like using u128s internally.

Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

ACK 1518517

@Kixunil Kixunil removed help wanted one ack PRs that have one ACK, so one more can progress them labels Dec 7, 2021
@JeremyRubin
Copy link
Copy Markdown
Contributor Author

yeah to be clear i personally don't think this patch is needed, so it's really up to others if they think it's needed.

@dr-orlovsky dr-orlovsky merged commit 9ae0f05 into rust-bitcoin:master Dec 11, 2021
@dr-orlovsky
Copy link
Copy Markdown
Collaborator

Merging, since it has two ACKs for some time already and is quite trivial

@JeremyRubin
Copy link
Copy Markdown
Contributor Author

I would like a post-merge ack i think maybe from @sanket1729 and @apoelstra if possible.

This was a follow up because there was some dissent on my use of u128 in a previous PR.

I know I authored it but i am neutral on this so i don't think the usual 2 acks works since usualy 2 acks is really 3, including the author's.

@apoelstra
Copy link
Copy Markdown
Member

yep, concept ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants