PSBT comparison problems #4752
Merged
PSBT comparison problems #4752
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
niftynei
force-pushed
the
nifty/psbt-matching-problem
branch
from
f815087
to
ad1bd9d
Compare
rustyrussell
reviewed
Sep 1, 2021
common/psbt_open.c
Outdated
Comment on lines
495
to
497
| bool psbt_contribs_changed(const tal_t *ctx, | ||
| struct wally_psbt *orig, | ||
| struct wally_psbt *new) | ||
| { |
There was a problem hiding this comment.
This is an antipattern! It doesn't return an allocated object, so why pass in a ctx?
I think you meant to use tmpctx internally.
There was a problem hiding this comment.
ACK, yes i can do that. Since this is common it seems .. better? to use an internal allocation/free cycle -- is that the wrong intuition?
There was a problem hiding this comment.
Hmm, tmpctx is pretty universal now. Either way.
Also remove all the unused code (checking for differences for a side etc)
Otherwise libwally pushes the psbt-key for 'witness script' onto the
serialized version and we fail the 'is this identical' check.
Relevant line from libwally, where if bytes, we push a psbt_key.
```
static void push_typed_varbuff(unsigned char **cursor, size_t *max,
uint64_t type,
const unsigned char *bytes, size_t bytes_len)
{
if (bytes) {
push_psbt_key(cursor, max, type, NULL, 0);
push_varbuff(cursor, max, bytes, bytes_len);
}
}
```
Reported-By: @grubles
Changelog-Fixed: openchannel_signed would fail on PSBT comparison of materially identical PSBTs
niftynei
force-pushed
the
nifty/psbt-matching-problem
branch
from
ad1bd9d
to
1fb4e58
Compare
rustyrussell
approved these changes
Sep 2, 2021
| * @new - 'updated' psbt, to verify is unchanged | ||
| */ | ||
| bool psbt_contribs_changed(struct wally_psbt *orig, | ||
| struct wally_psbt *new); |
There was a problem hiding this comment.
These should also be const struct wally_psbt, no?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@grubles found a bug where our PSBT 'is identical' code was failing -- the updated version included a witness script, whereas the original did not.
libwallypushes a type-byte onto the serialized version if there's any bytes at all -- setting the bytes to null avoids this.