Skip to content

Expand documentation and fields in SpendableOutputDescriptors #786

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Feb 16, 2021
Merged

Expand documentation and fields in SpendableOutputDescriptors #786

merged 5 commits into from
Feb 16, 2021

Conversation

TheBlueMatt
Copy link
Collaborator

This is pretty trivial, but not having channel_value_satoshis in the output descriptors really sucks.

@TheBlueMatt TheBlueMatt force-pushed the 2021-02-chansigner-util branch from 0fabfbc to 9d566cc Compare February 2, 2021 22:14
@codecov
Copy link

codecov bot commented Feb 2, 2021 •
edited
Loading

Codecov Report

Merging #786 (ed3f026) into main (879e309) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #786      +/-   ##
==========================================
- Coverage   90.84%   90.79%   -0.05%     
==========================================
  Files          44       44              
  Lines       24073    24084      +11     
==========================================
- Hits        21868    21866       -2     
- Misses       2205     2218      +13
Impacted Files Coverage Δ
lightning/src/chain/channelmonitor.rs 95.68% <100.00%> (+0.01%) ⬆️
lightning/src/chain/keysinterface.rs 92.55% <100.00%> (+0.15%) ⬆️
lightning/src/ln/channel.rs 87.26% <100.00%> (ø)
lightning/src/ln/functional_tests.rs 96.94% <100.00%> (-0.23%) ⬇️
lightning/src/util/enforcing_trait_impls.rs 90.09% <100.00%> (ø)
lightning/src/util/test_utils.rs 84.81% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 879e309...06342a3. Read the comment docs.

ariard
ariard reviewed Feb 2, 2021
View reviewed changes
lightning/src/ln/functional_tests.rs
@@ -7484,7 +7486,7 @@ fn test_data_loss_protect() {
let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42};
connect_block(&nodes[0], &Block { header, txdata: vec![node_txn[0].clone()]}, 0);
connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1, 0, true, header.block_hash());
let spend_txn = check_spendable_outputs!(nodes[0], 1, node_cfgs[0].keys_manager, 100000);
let spend_txn = check_spendable_outputs!(nodes[0], 1, node_cfgs[0].keys_manager, 1000000);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we don't have introduced failures ? Is this change relevant ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value checks just check that fee is >= 0, so passing in a low value here just resulted in the fee/weight calculation checks always passing.

@TheBlueMatt TheBlueMatt force-pushed the 2021-02-chansigner-util branch from 9d566cc to aa5768b Compare February 2, 2021 23:35
@devrandom
Copy link
Member

How do we expect the user to use channel_value_satoshis?

@TheBlueMatt
Copy link
Collaborator Author

Its required to reconstruct the InMemoryChannelKeys from a KeysManager - https://docs.rs/lightning/0.0.12/lightning/chain/keysinterface/struct.KeysManager.html#method.derive_channel_keys

@devrandom
Copy link
Member

Ah, yes, of course.

@TheBlueMatt TheBlueMatt mentioned this pull request Feb 4, 2021
Merged
@TheBlueMatt TheBlueMatt force-pushed the 2021-02-chansigner-util branch from aa5768b to b4d790c Compare February 6, 2021 19:45
@TheBlueMatt
Copy link
Collaborator Author

@devrandom took your suggestion from #789 (comment). Also included a bindings update commit, cause its not hard and its nice to land at the same time.

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 6, 2021
@TheBlueMatt
f rebase on new changes in lightningdevkit#786
8e78652
@TheBlueMatt TheBlueMatt force-pushed the 2021-02-chansigner-util branch from b4d790c to 00b80e9 Compare February 6, 2021 20:00
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Feb 6, 2021
@TheBlueMatt
f rebase on new changes in lightningdevkit#786
40773df
devrandom
devrandom approved these changes Feb 8, 2021
View reviewed changes
Copy link
Member

@devrandom devrandom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK (except for bindings, which I'm not very familiar with, yet)

@TheBlueMatt TheBlueMatt force-pushed the 2021-02-chansigner-util branch from 00b80e9 to 28027a3 Compare February 8, 2021 18:47
@TheBlueMatt
Swap key_derivation_params (u64, u64) for channel_keys_id [u8; 32]
e885d0a 
Instead of `key_derivation_params` being a rather strange type, we
call it `channel_keys_id` and give it a generic 32 byte array. This
should be much clearer for users and also more flexible.
@TheBlueMatt
Expand documentation and fields in SpendableOutputDescriptors
36cc581 
This adds a channel_value_satoshis field to
SpendableOutputDescriptors as it is required to recreate our
InMemoryChannelKeys. It also slightly expands documentation.
@TheBlueMatt
Slightly expand documentation on KeysInterface
7dbced3 
Specifically, this notes when methods can or can not return the
same value on each call.
@TheBlueMatt
Update auto-generated bindings
e9d819d
@TheBlueMatt
Adapt C++ bindings demo to new API and keys (new funding tx)
06342a3
@TheBlueMatt TheBlueMatt force-pushed the 2021-02-chansigner-util branch from 28027a3 to 06342a3 Compare February 12, 2021 23:59
@TheBlueMatt
Copy link
Collaborator Author

Rebased, only changes were to re-run the auto-bindings-generation.

@valentinewallace valentinewallace self-requested a review February 16, 2021 17:09
valentinewallace
valentinewallace approved these changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💯 nice docs improvements

@TheBlueMatt TheBlueMatt merged commit 2088f4b into lightningdevkit:main Feb 16, 2021
valentinewallace
valentinewallace reviewed Feb 16, 2021
View reviewed changes
lightning-c-bindings/demo.cpp
0x01, 0x40, 0x9c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x22, 0x00, 0x20, 0x20, 0x12, 0x70, 0x44,
0x41, 0x40, 0xaf, 0xc5, 0x72, 0x97, 0xc8, 0x69, 0xba, 0x04, 0xdb, 0x28, 0x7b, 0xd7, 0x32, 0x07,
0x33, 0x3a, 0x4a, 0xc2, 0xc5, 0x56, 0x06, 0x05, 0x65, 0xd7, 0xa8, 0xcf, 0x01, 0x00, 0x00, 0x00,
0x01, 0x40, 0x9c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x22, 0x00, 0x20, 0xd1, 0xd9, 0x13, 0xa9,
Copy link
Contributor

@valentinewallace valentinewallace Feb 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops post-merge review but, I don't get why this PR's changes would result in a different channel-opening tx being necessary in the demo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valentinewallace valentinewallace valentinewallace approved these changes

@ariard ariard ariard left review comments

@devrandom devrandom devrandom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TheBlueMatt @devrandom @valentinewallace @ariard