Skip to content
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

Decrease Huffman Weights to u32 #701

Merged
merged 1 commit into from Dec 11, 2021

Conversation

JeremyRubin
Copy link
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
Contributor Author

rebased :)

Copy link
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
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
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
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
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
Collaborator

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

@JeremyRubin
Copy link
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
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.

None yet

4 participants