Review updates by jgriffiths · Pull Request #210 · ElementsProject/libwally-core

jgriffiths · 2020-07-14T22:09:18Z

A large set of fixes and updates encompassing:

Review and bug fixing of changes over the last year
Documentation improvements
Standardising of interfaces and consistency of implementation
Improved tests
Merging all outstanding PSBT changes required for current c-lightning master
Additional PSBT fixes and cleanups
Wrapper updates

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

Doesn't affect functionality since SHA256_LEN is the same size, but this reads better.

…RRAY

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

Exposes the types used by the setters and renames some things so the existing SWIG pattern matches work for types.

There were no calls that take these functions so removing them should not cause any issues. Modifying the PSBTs transaction inputs and outputs will be added, which like psbt_set_global_tx() will update the PSBT to reflect the tx status. Otherwise for all other operations the caller will only need to set values within the existing PSBT inputs and outputs. This will save a lot of boilerplate and wrapping code from being required, and fits with e.g. c-lightnings view of how PSBTs will be manipulated.

SWIG wrappers should use the indexed setters exposed through swig_int.h to set values through the psbt, since they can't get pointers to the input and output items.

This will be even cleaner once pubkey is converted to a ptr + length from its current array form.

These should have followed the model of the transaction code; when Elements support is compiled in, inputs and outputs *are* Elements inputs and outputs and elements members can be set like any other members.

The previous interface was unusable: no users should be affected, so we just note that the new interface is final to fulfill the contract we have w.r.t. updating CHANGES.md for API changes.

LeoComandini · 2020-07-22T16:12:43Z

I went through the changed code and it looks good to me.
As said in the above comments other PRs will follow to cover some issues, add the missing tests and finalize the psbt interface.
So IMHO we should be good in merging this PR at this stage.

utACK 1b61666

The incorrect constant has the same size so this isn't an API change; it just makes the code a little clearer.

This required changes to paramater names to match the transaction fields, and includes documentation fixes and length checks for the setter functions.

Also fix innteger setters to go through the setter function to get proper error checking.

Note the definition of EC_FLAGS_ALL uses the hex values as the JS bindings require this to compile.

libsecp always produces low-s signatures so this isn't required.

LeoComandini · 2020-07-24T07:58:47Z

New changes looks good too,

utACK c9c45b6

jgriffiths added 30 commits July 6, 2020 08:19

Use WALLY_TXHASH_LEN for tx hash lengths.

79d8fad

Doesn't affect functionality since SHA256_LEN is the same size, but this reads better.

transaction: Use GET_TX_B_FIXED instead of mostly duplicated GET_TX_A…

fabb45b

…RRAY

Java: Add missing tx_input_get_txhash wrapper

995245d

blech32: Add witness version check as per bech32

bbb17be

blech32: Formatting consistency nits

eebe38a

pytest: Don't use built-in function 'input' as a variable name

1ddc85a

pytest: Remove hashlib use from test_sign.py

897768f

Docs: received -> receive

dffd215

docs: Add symmetric docs with minor fixes

3ebb7d9

symmetric: Add java wrappers + test, pass given len

d8bacc3

Also update version to uin32_t (currently always passed as 0).

wally_asset_blinding_key_from_seed: remove redundant goto

01c690f

wally_asset_blinding_key_to_ec_private_key: simplify logic

4574c82

docs: Fix function refs in Liquid developer guide

9ae32e9

docs: Fix sphinx warning

aebd386

elements: Remove redundant goto's

3a1f622

tests: Actually check pubkey recovery results

4280519

docs: Fix sphinx warning

5d7f840

ecdh: Use priv_key for param name, fix docs, expose nicely to Java

91b84a0

elements: Use wally for hashing in get_nonce_hash()

fe605a6

SWIG: Add missing nonce_hash to Java bindings

1003309

SWIG: Silence JS wrapper build warnings

a281961

tests: Remove hashbangs from python tests

d2c5d2c

address.c: Remove unused includes

73cdf0a

wally_bip32_key_to_addr_segwit: Clear witness program before returning

2203ec3

SWIG: Java wrapper and test for ec_public_key_negate()

118cc76

Compare return codes to WALLY_OK for consistency

333edf1

pegin_contract_script_from_bytes: Don't use 'written' as a general va…

52cdf7c

…riable This ensures we leave written as 0 in the case of an error.

address_to_scriptpubkey: Use common variable name for consistency

e312d47

scriptpubkey_to_address: Don't ignore base58 errors

5af7358

For example, output being NULL should return WALLY_EINVAL.

jgriffiths added 12 commits July 23, 2020 00:46

tests: fix clang warning

1abd2a0

psbt: Windows build fixes

71ffd97

psbt: Add non-elements swig setters, with tests

384947f

Exposes the types used by the setters and renames some things so the existing SWIG pattern matches work for types.

symmetric: Add missing symmetric header to install list

204419a

psbt: Don't expose psbt input/output direct setters to SWIG

ee4f030

SWIG wrappers should use the indexed setters exposed through swig_int.h to set values through the psbt, since they can't get pointers to the input and output items.

psbt: push pubkey keys directly

d1ebc72

This will be even cleaner once pubkey is converted to a ptr + length from its current array form.

psbt: Add a FIXME for adding validation to getters/setters

c1ea199

psbt: Take the version in psbt_init_alloc for future compatibility

d74863e

psbt: use BIP32_KEY_FINGERPRINT_LEN per review

36ed6c8

psbt: rename elements input/output getters/setters

b9b1897

These should have followed the model of the transaction code; when Elements support is compiled in, inputs and outputs *are* Elements inputs and outputs and elements members can be set like any other members.

psbt: Update CHANGES.md to a simpler message for the next release

1b61666

The previous interface was unusable: no users should be affected, so we just note that the new interface is final to fulfill the contract we have w.r.t. updating CHANGES.md for API changes.

jgriffiths force-pushed the review_updates branch from 559746c to 1b61666 Compare July 22, 2020 12:48

LeoComandini reviewed Jul 22, 2020

View reviewed changes

Comment thread src/psbt.c

jgriffiths self-assigned this Jul 22, 2020

jgriffiths added 2 commits July 23, 2020 08:08

transaction: update docs for allowed value lengths

6a9b309

elements: use the correct constant for blinding factor lengths

87e45e0

The incorrect constant has the same size so this isn't an API change; it just makes the code a little clearer.

jgriffiths force-pushed the review_updates branch from 834316f to 6adbd24 Compare July 22, 2020 22:17

jgriffiths added 3 commits July 23, 2020 10:50

psbt: Expose output setters to python, with tests

24e4db9

This required changes to paramater names to match the transaction fields, and includes documentation fixes and length checks for the setter functions.

psbt: Simplify input/output pull logic

d20c66b

psbt: cleanup code for and validate field lengths when reading in psbts

846a4d8

jgriffiths force-pushed the review_updates branch from 6adbd24 to 846a4d8 Compare July 22, 2020 22:50

psbt: Expose input setters to python, with tests

f6ef719

Also fix innteger setters to go through the setter function to get proper error checking.

jgriffiths force-pushed the review_updates branch from f2a8cba to f6ef719 Compare July 23, 2020 13:11

jgriffiths added 2 commits July 24, 2020 01:41

psbt: Allow the caller to control sig grinding when signing

b396b85

Note the definition of EC_FLAGS_ALL uses the hex values as the JS bindings require this to compile.

psbt: Don't normalise signatures after signing

c9c45b6

libsecp always produces low-s signatures so this isn't required.

jgriffiths merged commit c9c45b6 into master Jul 26, 2020

jgriffiths deleted the review_updates branch July 26, 2020 02:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Review updates - #210

Review updates#210
jgriffiths merged 205 commits into
masterfrom
review_updates

jgriffiths commented Jul 14, 2020 •

edited

Loading

Uh oh!

Uh oh!

LeoComandini commented Jul 22, 2020

Uh oh!

LeoComandini commented Jul 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

jgriffiths commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

LeoComandini commented Jul 22, 2020

Uh oh!

LeoComandini commented Jul 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jgriffiths commented Jul 14, 2020 •

edited

Loading