Review updates and PSBT changes - #209
Merged
jgriffiths merged 165 commits intoJul 14, 2020
Merged
Conversation
Doesn't affect functionality since SHA256_LEN is the same size, but this reads better.
…heck Checked in here to not lose it until this is pushed upstream. Wally only uses v0 addresses at the time of commiting. Note that the inverse function segwit_addr_decode() isn't affected as it implicitly casts uint8 -> int32 so witver cannot be negative on return.
Also update version to uin32_t (currently always passed as 0).
…riable This ensures we leave written as 0 in the case of an error.
For example, output being NULL should return WALLY_EINVAL.
The convention is to return WALLY_OK and put the required length in *written. The caller must check that the returned amount fits within the buffer they supplied. The SWIG generated wrappers will throw an exception on any return result except WALLY_OK. This and other library conventions will be documented shortly.
There is plenty more to be documented, but this at least documents the most common user confusion regarding variable length output buffers and how they are handled.
based on a patch from niftynei.
Regarding handling duplicated keys, BIP 174 states: Keys within each scope should never be duplicated; all keys in the format are unique. PSBTs containing duplicate keys are invalid. However implementors will still need to handle events where keys are duplicated when combining transactions with duplicated fields. In this event, the software may choose whichever value it wishes. If the caller wishes to return an error when adding duplicate keys, they should use the find functions to check for duplicates before adding values.
Also clean up the signing code a little
There is no way to create an array of psbts using the current wally interface - since only wally_psbt_init_alloc() is exposed, you'd have be doing something nasty like using memcpy to copy structs into an array etc (which the ctypes python test was actually doing). Instead of this nastyness, just let the caller loop through their inputs and merge them in one by one. The code for this is significantly cleaner too.
In particular, there is no need to search for duplicates now that the add code follows the BIP semantics.
Also fix sighash combining to only overwrite if no sighash is given in the destination PSBT.
This reverts commit 211df9d. The previous behaviour is what is intended as per the newly updated docs.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
A large set of fixes and updates encompassing:
Although there remains a significant amount of PSBT work to complete, I think this is worth merging now so that the lightning peeps can move back to wally master instead of their hacked/blessed branch, and further changes can be smaller and more easily reviewable.
Obsoletes or includes the following PRs: #208 #207 #206 #204 #201 #199 #198 #194 #193 #191
Fixes #205
Fixes #189
Fixes #184
Fixes #139