-
Notifications
You must be signed in to change notification settings - Fork 143
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
use max_weight_to_satisfy instead of deprecated max_satisfaction_weight #647
Conversation
Much of the discussion on #476 is about the fact that we changed the definition of "max satisfaciton weight". This is also why we deprecated and renamed the function rather than changing it in place. It wasn't "wrong" before, but it also was computing something different that we struggled to explain and support. If you look at the definition of I believe the old method was also including the segwit marker byte (4wu) and the length of the witness stack in case the satisfaction somehow switches the input from non-segwit to segwit (1wu). But I'm unsure. In any case you can read the code and see that the old version has a |
There aren't unit test failing here, so I will just undraft this, thanks for explaining |
Descriptor::Wpkh(ref wpkh) => wpkh.max_weight_to_satisfy(), | ||
Descriptor::Wsh(ref wsh) => wsh.max_weight_to_satisfy()?, | ||
Descriptor::Sh(ref sh) => sh.max_weight_to_satisfy()?, | ||
Descriptor::Tr(ref tr) => tr.max_weight_to_satisfy()?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In d501a13:
Lol, this changes the behavior of the function. We cannot make this change without silently breaking people.
Sorry, I just read the diff. concept NACK. The point of deprecating and renaming these functions was that we would signal to users that they need to move to the new one. If we change the functions then we just silently break them. If your concern is the deprecation warnings then you should whitelist the warnings for these functions. Pretty stupid that Rust is complaining about calling deprecated functions from deprecated functions.. |
Ah I see, what about #648 ? |
b96a1b5 allow deprecated method calls in deprecated methods (Riccardo Casatta) Pull request description: instead of #647 ACKs for top commit: apoelstra: ACK b96a1b5 Tree-SHA512: 9afe29493db61e1a1251adcec286ed23fc989f6379e28476f8e38624027b14bc29922385f2b8baa1f2fec93afe534c8e68d429f8619c01be751b3f44c65b33b7
This was simply meant to get rid of the warnings but I put in draft because it seems clear the two method are not equivalent for wpkh, with the new method computing always 5 less than the old one. Maybe it was simply wrong before but I can't explain this line:
Which is always zero
Disclosure: I didn't read all #476