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

use max_weight_to_satisfy instead of deprecated max_satisfaction_weight #647

Closed
wants to merge 1 commit into from

Conversation

RCasatta
Copy link
Contributor

@RCasatta RCasatta commented Mar 1, 2024

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:

        let stack_varint_diff = varint_len(2) - varint_len(0);

Which is always zero

Disclosure: I didn't read all #476

@apoelstra
Copy link
Member

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 stack_varint_diff for all the different output types you will see that they all use the same formula except that 2 is changed to be the maximum number of stack elements. Yes, for wpkh outputs the value is always 0, but it's clearer to compute this the same way for every output so it's clear that we're computing the same thing. Which is the cost of the length prefix for this input's witness stack.

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 4 + 1 + where the new one does not. So you can just adjust all the unit tests by 5.

@RCasatta
Copy link
Contributor Author

RCasatta commented Mar 1, 2024

So you can just adjust all the unit tests by 5

There aren't unit test failing here, so I will just undraft this, thanks for explaining

@RCasatta RCasatta marked this pull request as ready for review March 1, 2024 14:45
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()?,
Copy link
Member

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.

@apoelstra
Copy link
Member

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..

@RCasatta
Copy link
Contributor Author

RCasatta commented Mar 1, 2024

Ah I see, what about #648 ?

@RCasatta RCasatta closed this Mar 1, 2024
apoelstra added a commit that referenced this pull request Mar 1, 2024
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
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.

2 participants