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

Update emergency.recover after every 'commitment_revocation' so that user can sweep funds by penalty transaction. #7772

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

adi2011
Copy link
Collaborator

@adi2011 adi2011 commented Oct 31, 2024

This PR would enable nodes to create penalty transaction when the peer publishes an old revoked state using the emergency.recover.

Addresses: #7696

@adi2011 adi2011 marked this pull request as ready for review November 4, 2024 18:55
@adi2011 adi2011 force-pushed the enablepenaltytxn branch 2 times, most recently from 6c70397 to eeb239f Compare November 4, 2024 19:27
@rustyrussell rustyrussell added this to the v24.11 milestone Nov 11, 2024
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Nice! Only one minor cleanup suggestion.

I'll rebase now...

wallet/wallet.c Outdated

for (u32 pos = 0; pos < chain->chain.num_valid; pos++) {
struct secret s;
memcpy(&s, &chain->chain.known[pos].hash, sizeof(s));
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a new macro for, this: CROSS_TYPE_ASSIGNMENT(&s, &chain->chain.known[pos].hash). It checks the two types are same size...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed it. Thanks!!

@rustyrussell
Copy link
Contributor

Rebased on master with simple API fixes.

@adi2011 adi2011 force-pushed the enablepenaltytxn branch 2 times, most recently from 84bebbf to 7f13d7b Compare November 19, 2024 11:59
@rustyrussell
Copy link
Contributor

This doesn't work for old scb files (as you can tell, from the test!)

It should...

@adi2011 adi2011 force-pushed the enablepenaltytxn branch 2 times, most recently from 4312f07 to 51d0031 Compare November 28, 2024 22:22
@adi2011 adi2011 requested a review from rustyrussell November 28, 2024 22:23
This change will allow subtypes in wiregen files to have tlvstreams.
Shifting tlv structs above subtypes in header_template is done to prevent
forward declaration.
Since generate-wire prepends 'tlv_' in tlvname, we
have to modify fromwire_subtype_field and towire_subtype_field in
impl_template to accommodate this.
…rgencyrecover to create penalty txn when peer publishes an old revoked state.
…chan()

- Added  field to store commitment secret chain in
- Added  field to hold counterparty's basepoint information
- Added  field to indicate the channel initiator's side (local or remote)

These fields enhance  to make channels which are capable to create penalty transaction in case the peer broadcasts an old revoked state.
…directly to the database

- Adds 'wallet_stub_shachain_init' function to store a retrieved shachain in the database.
- Saves initial metadata ('min_index' and 'num_valid') to the 'shachains' table.
- Iterates through known shachain secrets, adding each entry to 'shachain_known' with position, index, and hash.

This function enables direct persistence of shachain data, ensuring all relevant information is saved upon retrieval.
…pdate emergency.recover everytime we get a revocation secret.
Add test to check if we can successfully create a penalty
transaction when we recover a channel using emergencyrecover
and the peer publishes an old state.

Changelog-Added: Now the emergency.recover file would keep track of all the revocation secrets when any channel advances it's state.
Copy link
Contributor

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

seems like a sensible and reasonable change. the commits were very easy to read through, kudos on that!

very cool that we're basically turning peers into watchtowers for ourselves.

#include <common/wireaddr.h>
#include <common/channel_type.h>
#include <common/derive_basepoints.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: list these alphabetically?

Copy link
Contributor

Choose a reason for hiding this comment

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

for commit 53b16a0, it looks like a keyword got omitted from the commit message

@@ -1465,9 +1469,13 @@ static struct channel *stub_chan(struct command *cmd,
struct pubkey localFundingPubkey;
struct pubkey pk;
struct short_channel_id *scid;
u32 blockht;
u32 blockht = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

why set this here? it doesn't look like it got used again in this commit

@@ -2368,6 +2368,8 @@ static void json_add_scb(struct command *cmd,
{
u8 *scb = tal_arr(cmd, u8, 0);

// Update shachain in SCB.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: comments are of form /* Update shachain in SCB. */

@@ -1013,6 +1013,9 @@ void tell_connectd_peer_importance(struct peer *peer UNNEEDED,
/* Generated stub for tlv_hsmd_dev_preinit_tlvs_new */
struct tlv_hsmd_dev_preinit_tlvs *tlv_hsmd_dev_preinit_tlvs_new(const tal_t *ctx UNNEEDED)
{ fprintf(stderr, "tlv_hsmd_dev_preinit_tlvs_new called!\n"); abort(); }
/* Generated stub for tlv_scb_tlvs_new */
struct tlv_scb_tlvs *tlv_scb_tlvs_new(const tal_t *ctx UNNEEDED)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: squash this into the commit that adds this new method!

@rustyrussell rustyrussell modified the milestones: v24.11, v25.02 Dec 13, 2024
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.

3 participants