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

Static Channel Backup. #5288

Closed
wants to merge 0 commits into from
Closed

Conversation

adi2011
Copy link
Collaborator

@adi2011 adi2011 commented May 24, 2022

SCB will enable users to backup channels in an encrypted file emergency.recover, and recover funds upon data corruption. I've added some RPCs and modifications to the in-memory channel struct to enable this.

-staticbackup: Returns SCB (in the form of a TLV) of all the existing channels.

-recoverchannel: Commits channels (with enough data to recover funds and the rest is dummy data) to the DB.

-makesecret: Gives out pseudorandom keys (based on the hex input) derived from the HSM.

-emergencyrecover: Recover all the stored channels in case of data loss

There's a new TLV message as well (scb_wire) which is used to serialize/deserialize the data for a channel.
The scb file is maintained by a plugin.

Thanks, @niftynei, and @rustyrussell for all the valuable inputs and guidance. ❤️

@adi2011 adi2011 force-pushed the master branch 2 times, most recently from 7359161 to f84dd4b Compare May 25, 2022 07:42
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Excellent work @adi2011, I saw your PR and got excited to take a look. I know this is just a draft, but it's already looking very good.

Since I was here I added a couple of comments, mostly unimportant stuff like formatting (hadn't realized the Draft status) so feel free to ignore them as you likely would have addressed them before undrafting.

One bikeshedding topic I'd like to tackle though is the name SCB, as it always annoyed me that it is being positioned as a backup. Would you be open to call it "emergency recovery" or something like that?

@@ -146,6 +147,43 @@ static struct command_result *json_getsharedsecret(struct command *cmd,
return command_success(cmd, response);
}

static struct command_result *json_getsecret(struct command *cmd,
Copy link
Member

Choose a reason for hiding this comment

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

Is this not already achieved by getsharedsecret? I'm always a bit hesitant to add generic "give me a secret" methods as they may lead to extracting secrets we don't want to give away.

Copy link
Collaborator Author

@adi2011 adi2011 May 25, 2022

Choose a reason for hiding this comment

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

getsharedsecret needs a compressed public key and then it performs ecdh() to give out a key for connection handshake and for onion wrapping.

We needed a pseudorandom key derived from hsm_secret, using hkdf_sha256() taking in a string as info, but I get your point.
what if instead of taking in an 'info' string from the user I hard code it to "peer backup" to get a specific key used to encrypt the scb file? and rename it to getbackupsecret?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ummmm just generate a random compressed public key? Pick one out of a hat, then call getsharedsecret and then use the result as the encryption key, AND store the compressed public key in the first 33 bytes of the emergency-recovery file. This lets you recover it anyway with the same hsm_secret; you only need the session pubkey.

Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj May 31, 2022

Choose a reason for hiding this comment

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

Secondarily, the VLS people have implemented a hardware version of the hsmd, and adding a new API to the hsmd requires that we at least inform them so they can implement as well. So I would be reluctant to add a new hsmd_derive_secret message here, we should try to use getsharedsecret which they already implemented (hsmd_ecdh_req, hsmd_ecdh_resp) and just give a random point that you store with the recovery file anyway. The random point will not leak the encryption key because of ECDH magic so it is safe to put it right beside the encrypted blob in the recovery file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've modified it by deriving a key from the HSM and then derived the keys for this RPC from that one, I don't think that'd cause any security issue and this can be used for generating keys for other things as well in the future.

Comment on lines 454 to 455
}
else{
Copy link
Member

Choose a reason for hiding this comment

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

Nit: whitespace -> } else {

channel->last_tx = tal_steal(channel, last_tx);
channel->last_tx->chainparams = chainparams;
channel->last_tx_type = TX_UNKNOWN;
if(last_tx){
Copy link
Member

Choose a reason for hiding this comment

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

Nit: whitespace: if (last_tx) {

}

static const struct json_command hex_scb_command = {
"hex-scb",
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use - in command names, they translate very poorly into other languages, requiring workarounds and renaming. We usually just go with something like hexscb. Also it'd be nicer to have staticbackup as a name instead, hex just being the format, allowing us to iterate in the future, without requiring a rename.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad, Yes I'll change it to staticbackup...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe static_backup is better? it makes the docs more accessible for screen readers too.

But usually, the _ will translate directly into other languages with the camel case, so it is more difficult for single word command name.

But it is minor if it requires a lot of work!

@@ -756,6 +756,14 @@ void channel_fail_permanent(struct channel *channel,
const char *fmt,
...)
{
/* FIXME: should I compare the whole scid for stub channel?
I doubt there are actually any channels in the Genesis block(1x1x1) */
if(channel->scid->u64 >> 40 == 1){
Copy link
Member

Choose a reason for hiding this comment

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

Is 1x1x1 a special value in this case? We're not actually guaranteed to have an scid at the time we create the recovery data, since the channel may not have yet confirmed.

Copy link
Member

Choose a reason for hiding this comment

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

I see it below, seems a bit strange to use this, and not a way more unreasonable UINT64_MAX, but it'd ultimately be the same anyway. Maybe it'd be better to just keep it NULL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am setting it to 1x1x1 to indicate (while loading the in-memory channel from DB) that it's a stub channel and we need to send the error to the peer, nothing else. It was on my to-do list to set it to 0x0x0 (as Rusty suggested), but it won't make any difference.

I think setting it to NULL will create confusion with the unconfirmed channels which also don't have any scid? If that's not the case I am happy to set it to NULL.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the idea to make the code unambiguous for the new readers, maybe we can make a #DEFINE GENESIS_SHORT_ID "1x1x1"

P.S: I'm assuming that it is easy to convert into a string the channel->scid

Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to remember to read the PR from bottom to top, the Rusty suggestion is better I think #5288 (comment)

lightningd/opening_control.c Outdated Show resolved Hide resolved
Comment on lines 1278 to 1280
AMOUNT_SAT(500000000), // FIXME: Is this correct?
/*Need to keep this very high from trimming the fees
in onchaind.c(main).*/
Copy link
Member

Choose a reason for hiding this comment

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

Might be worthwile to add the actual funding amount to the recovery data? It's not going to change anyway, so it'd be safe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I thought of adding it, but then, these channels will be unilaterally closed by the peer and the wallet will forget them after sweeping the funds. So I don't think there's any point in swelling up the SCB file with unnecessary info.

How about setting it to an unrealistically high number?

Copy link
Collaborator

Choose a reason for hiding this comment

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

2,100,000,000,000,001 satoshis?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

2,100,000,000,000,001 satoshis?

Fixed in later commits.

@@ -1227,3 +1406,13 @@ static const struct json_command fundchannel_complete_command = {
"with {psbt}. Returns true on success, false otherwise."
};
AUTODATA(json_command, &fundchannel_complete_command);

static const struct json_command json_commitchan_command = {
"emergencystubchannel",
Copy link
Member

Choose a reason for hiding this comment

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

recoverchannel?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I am anyways bad at naming things in general...

wallet/wallet.c Outdated
that can motivate him to cheat.*/
chan->error = towire_errorfmt(w->ld,
&cid,
"We can't be together anymore.."
Copy link
Member

Choose a reason for hiding this comment

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

I like this error message 👍

}

/* checks if the SCB file exists, creates a new one in case it doesn't. */
static void maybe_create_new_scb(struct plugin *p, const char *scb_buf)
Copy link
Member

Choose a reason for hiding this comment

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

This overwrites the file in place, which does not guarantee atomic operations. It's better to create a temporary file recover.tmp, write to that and then rename recover.tmp to recover. That guarantees that a partial write (plugin crash, loss of power, etc) doesn't corrupt your recovery file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that was my doubt as well. I'll do this right away 🙌, Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@adi2011
Copy link
Collaborator Author

adi2011 commented May 25, 2022

Excellent work @adi2011, I saw your PR and got excited to take a look. I know this is just a draft, but it's already looking very good.

Thank you so much for taking out the time to check this out. I'll soon remove the draft tag after testing notification responses of the plugin, looking forward to more valuable feedback 😄

One bikeshedding topic I'd like to tackle though is the name SCB, as it always annoyed me that it is being positioned as a backup. Would you be open to call it "emergency recovery" or something like that?

Yes, I agree with this. Do I need to change the names of the TLV message and commands as well?

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.

OK, didn't review plugin yet! But lots of comments on the other stuff. Mostly easy things, though!

This is an excellent first PR :)

hsmd/libhsmd.c Outdated Show resolved Hide resolved
hsmd/libhsmd.c Outdated Show resolved Hide resolved
lightningd/hsm_control.c Outdated Show resolved Hide resolved
lightningd/hsm_control.c Outdated Show resolved Hide resolved
lightningd/channel.c Outdated Show resolved Hide resolved
lightningd/opening_control.c Outdated Show resolved Hide resolved
lightningd/opening_control.c Outdated Show resolved Hide resolved
lightningd/opening_control.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
@adi2011
Copy link
Collaborator Author

adi2011 commented May 25, 2022

Thank you so much for the review and sorry for the NIT:s (I'll keep them in mind from now onwards), I'll try to incorporate all the changes that you and @cdecker suggested by the end of this week.

@adi2011
Copy link
Collaborator Author

adi2011 commented May 27, 2022

I hope after sorting out the final commit's FIXME, this would be mergeable 🤞🏻 ...

@adi2011 adi2011 marked this pull request as ready for review May 28, 2022 11:30
@ZmnSCPxj
Copy link
Collaborator

Suggest adding a section to doc/BACKUP.md about this. In particular, since this now uses an atomic construct-temporary-file-and-rename, it is very safe for an automated backup process to hardlink the recovery file, copy the hardlinked name, then unlink, and the automated backup process assuredly will never acquire a corrupted copy, either.

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

I make the first review of this, so I can continue later.

basically are only C-style code fixes, and some json rpc command good manners when there is a failure.

@@ -90,6 +90,7 @@ LIGHTNINGD_COMMON_OBJS := \
common/features.o \
common/fee_states.o \
common/peer_status_wiregen.o \
common/scb_wiregen.o \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
common/scb_wiregen.o \
common/scb_wiregen.o \

we can alienate the \

Comment on lines 456 to 463
} else {
channel->last_tx = NULL;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
} else {
channel->last_tx = NULL;
}
} else
channel->last_tx = NULL;

Comment on lines 534 to 537
channel->error = towire_errorfmt(peer->ld,
&channel->cid,
"We can't be together anymore."
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is unformatted?

Comment on lines 768 to 770
if (is_stub_scid(channel->scid)) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (is_stub_scid(channel->scid)) {
return;
}
if (is_stub_scid(channel->scid))
return;

NULL))
return command_param_failed();

u8 *point = tal_dup_arr(cmd, u8, (u8 *)info, strlen(info), 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
u8 *point = tal_dup_arr(cmd, u8, (u8 *)info, strlen(info), 0);
u8 *point = tal_dup_arr(cmd, u8, (u8*)info, strlen(info), 0);

Comment on lines 75 to 120
if (fd < 0) {
/* If this is not the first time we've run, it will exist. */
if (errno == EEXIST)
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (fd < 0) {
/* If this is not the first time we've run, it will exist. */
if (errno == EEXIST)
return;
}
if (fd < 0)
/* If this is not the first time we've run, it will exist. */
if (errno == EEXIST)
return;

Comment on lines 91 to 139
if (fsync(fd) != 0) {
unlink_noerr("scb.tmp");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (fsync(fd) != 0) {
unlink_noerr("scb.tmp");
}
if (fsync(fd) != 0)
unlink_noerr("scb.tmp");

Comment on lines 97 to 146
if (close(fd) != 0) {
unlink_noerr("scb.tmp");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (close(fd) != 0) {
unlink_noerr("scb.tmp");
}
if (close(fd) != 0)
unlink_noerr("scb.tmp");

Comment on lines 104 to 106
if (fd < 0) {
plugin_log(p, LOG_DBG, "Opening: %s", strerror(errno));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (fd < 0) {
plugin_log(p, LOG_DBG, "Opening: %s", strerror(errno));
}
if (fd < 0)
plugin_log(p, LOG_DBG, "Opening: %s", strerror(errno));

after_recover_rpc,
&forward_error, NULL);

const jsmntok_t *restok = json_parse_simple(tmpctx, res, strlen(res));
Copy link
Collaborator

Choose a reason for hiding this comment

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

we decare this on top like const jsmntok_t *restok; and after

Suggested change
const jsmntok_t *restok = json_parse_simple(tmpctx, res, strlen(res));
restok = json_parse_simple(tmpctx, res, strlen(res));

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you @vincenzopalazzo for the review! I've incorporated these changes in the last fixup.

Comment on lines 2 to 16
#include <ccan/array_size/array_size.h>
#include <ccan/read_write_all/read_write_all.h>
#include <ccan/noerr/noerr.h>
#include <ccan/err/err.h>
#include <ccan/fdpass/fdpass.h>
#include <ccan/tal/str/str.h>
#include <ccan/json_out/json_out.h>
#include <ccan/cast/cast.h>
#include <ccan/mem/mem.h>
#include <ccan/str/hex/hex.h>
#include <common/json_tok.h>
#include <common/json.h>
#include <common/json_stream.h>
#include <common/hsm_encryption.h>
#include <common/type_to_string.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these includes are probably unnecessary? (e.g. ccan/fdpass/fdpass.h)?

plugins/chanbackup.c Outdated Show resolved Hide resolved
plugins/chanbackup.c Outdated Show resolved Hide resolved
plugins/chanbackup.c Outdated Show resolved Hide resolved
Comment on lines 75 to 76
if (fd < 0)
/* If this is not the first time we've run, it will exist. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Always use { if you have more than one statement inside (esp. another if statement!)

plugins/chanbackup.c Outdated Show resolved Hide resolved
Comment on lines 245 to 247
const char *scb_buf = json_strdup(tmpctx, buf, params);
const jsmntok_t *scb_arr = json_parse_simple(tmpctx, scb_buf, strlen(scb_buf)),
*scbs = json_get_member(scb_buf, scb_arr, "scb");
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just do const jmsntok_t *scbs = json_get_member(params, "scb"). You are converting the JSON to a string and reparsing it, which is unnecessary!

static void write_hsm(struct plugin *p, int fd,
const char *buf)
{
u8 *point = tal_dup_arr(buf, u8, (u8 *)buf, strlen(buf), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This routine encrypts the JSON, which is inefficient. Define a message containing an array of scbs in common/scb_wire.csv, and then use "towire_" and "fromwire_" to put it in the file.

Comment on lines 262 to 348
/* FIXME: I wanted to update the file on CHANNELD_AWAITING_LOCKIN,
* But I don't get update for it, maybe because there is no previous_state,
* also apparently `channel_opened` gets published when *peer* funded a channel with us?
* So, is their no way to get a notif on CHANNELD_AWAITING_LOCKIN? */
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, file a bug, this seems wrong!

* But I don't get update for it, maybe because there is no previous_state,
* also apparently `channel_opened` gets published when *peer* funded a channel with us?
* So, is their no way to get a notif on CHANNELD_AWAITING_LOCKIN? */
if(!strcmp(state ,"CLOSED") || !strcmp(state ,"CHANNELD_NORMAL")){
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use json_tok_streq() BTW. And needs a space before {

@adi2011 adi2011 force-pushed the master branch 2 times, most recently from f39b2fd to 5d2897e Compare June 17, 2022 09:23
@adi2011 adi2011 marked this pull request as draft June 23, 2022 22:38
@adi2011 adi2011 force-pushed the master branch 3 times, most recently from 184d257 to 4ed642f Compare June 24, 2022 07:05
@adi2011 adi2011 marked this pull request as ready for review June 24, 2022 07:07
@adi2011 adi2011 requested a review from niftynei June 26, 2022 06:17
@niftynei niftynei added this to the v0.12 milestone Jun 27, 2022
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.

This is really nice! No really big feedback, just some style things and naming.

Fix those and we can enable this. I've marked it for the v0.12 milestone!

Comment on lines 160 to 167
if (!param(cmd, buffer, params,
p_req("info", param_string, &info),
NULL))
return command_param_failed();

u8 *point = tal_dup_arr(cmd, u8, (u8*)info, strlen(info), 0);

u8 *msg = towire_hsmd_derive_secret(cmd, point);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we insist that info is in hex, or accept a string here? We could have two parameters (both p_opt, but make sure one is set!), called "string" and "hex": that's what datastore does.

NULL))
return command_param_failed();

u8 *point = tal_dup_arr(cmd, u8, (u8*)info, strlen(info), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

point is a weird name, BTW :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, changing it...

&json_getsecret,
"Get a pseudorandom secret key, using an info string."
};
AUTODATA(json_command, &getsecret_command);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also write doc/lightning-makesecret.7.md, and doc/schema/makesecret.request.json and doc/schema/makesecret.schema.json...

@@ -244,7 +244,7 @@ class Type(FieldSet):
'tx_parts',
'wally_psbt',
'wally_tx',
'channel_type',
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't delete channel_type here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a duplicate...

channel->scb->funding = *funding;
channel->scb->cid = *cid;
channel->scb->funding_sats = funding_sats;
channel->scb->type = (struct channel_type *) type;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use channel_type_dup(channel->scb, type) here: it makes sense for the scb to "own" the channel_type


return channel;
}
static struct command_result *json_commit_channel(struct command *cmd,
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to match the command name? i.e. json_recoverchannel.

#define VERSION ((u64)1)

/* Global secret object to keep the derived encryption key for the SCB */
struct secret secret;
Copy link
Contributor

Choose a reason for hiding this comment

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

static...

const char *scb_buf;
rpc_scan(p, "staticbackup",
take(json_out_obj(NULL, NULL, NULL)),
"{scb:%}", JSON_SCAN(json_to_str, &scb_buf));
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, converting to a string an back to JSON again is really ugly :(

Can you simply call staticbackup async here? Maybe create a function to wrap this:

		struct out_req *req;
		req = jsonrpc_request_start(plugin, NULL,
                                            "staticbackup",
                                            after_staticbackup,
                                            &forward_error,
                                            NULL);

		return send_outreq(plugin, req);

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I Applied 200% of my brain and created json_to_scb_chan, which makes it easier for writing it in the file as well...

Comment on lines 312 to 332
update_scb(cmd->plugin, json_strdup(tmpctx, buf, scbs));
return notification_handled(cmd);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hand this the buf and scbs token, and have it iterate through, rather than covnerting to a string and back...

This is neater, since scbs->size will tell it immediately how many scbs there are.

"recovery",
"Populates the DB with stub channels",
"returns stub channel-id's on completion",
recover,
Copy link
Contributor

Choose a reason for hiding this comment

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

I always like the function for a command to be json_COMMANDNAME, in this case json_emergencyrecover. That way I can easily find it!

@rustyrussell
Copy link
Contributor

Suggest adding a section to doc/BACKUP.md about this. In particular, since this now uses an atomic construct-temporary-file-and-rename, it is very safe for an automated backup process to hardlink the recovery file, copy the hardlinked name, then unlink, and the automated backup process assuredly will never acquire a corrupted copy, either.

It's also safe to just copy :) Every copy does open, read, close, which has the same semantic.

*channels = tok->size ? tal_arr(tmpctx, struct scb_chan *, tok->size) : NULL;

json_for_each_arr(i, t, tok) {
const u8 *scb_tmp = tal_hexdata(tmpctx, json_strdup(tmpctx, buffer, t),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Grossly violated the 80 col rule here...

@adi2011 adi2011 force-pushed the master branch 3 times, most recently from a5d88e3 to 63d85d3 Compare July 8, 2022 00:27
@niftynei
Copy link
Collaborator

niftynei commented Jul 8, 2022

Looks like there's a bug in test_emergencybackup

2022-07-08T00:53:35.357Z **BROKEN** lightningd: FATAL SIGNAL 11 (version c4fe1d3-modded)
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: common/daemon.c:38 (send_backtrace) 0x564000429f3e
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: common/daemon.c:46 (crashdump) 0x564000429f92
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: /build/glibc-SzIz7B/glibc-2.31/signal/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0 ((null)) 0x7ff33ef5e08f
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: ./bitcoin/short_channel_id.h:42 (is_stub_scid) 0x5640003a2bb0
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: lightningd/channel.c:789 (channel_fail_permanent) 0x5640003a575a
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: lightningd/onchain_control.c:623 (onchaind_funding_spent) 0x5640003d0496
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: lightningd/peer_control.c:1638 (funding_spent) 0x5640003e5fa9
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: lightningd/watch.c:271 (txowatch_fire) 0x5640004002d3
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: lightningd/chaintopology.c:87 (filter_block_txs) 0x56400039fc0d
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: lightningd/chaintopology.c:758 (add_tip) 0x5640003a1ac8
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: lightningd/chaintopology.c:850 (get_new_block) 0x5640003a2027
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: lightningd/bitcoind.c:388 (getrawblockbyheight_callback) 0x56400039e34f
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: lightningd/plugin.c:587 (plugin_response_handle) 0x5640003f44b1
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: lightningd/plugin.c:693 (plugin_read_json_one) 0x5640003f46da
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: lightningd/plugin.c:738 (plugin_read_json) 0x5640003f48be
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: ccan/ccan/io/io.c:59 (next_plan) 0x56400049b3ce
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: ccan/ccan/io/io.c:407 (do_plan) 0x56400049bfd6
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: ccan/ccan/io/io.c:417 (io_ready) 0x56400049c018
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: ccan/ccan/io/poll.c:453 (io_loop) 0x56400049e30b
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: lightningd/io_loop_with_timers.c:22 (io_loop_with_timers) 0x5640003c2ce4
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: lightningd/lightningd.c:1184 (main) 0x5640003c8fc8
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: ../csu/libc-start.c:308 (__libc_start_main) 0x7ff33ef3f082
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: (null):0 ((null)) 0x56400039d38d
2022-07-08T00:53:35.357Z **BROKEN** lightningd: backtrace: (null):0 ((null)) 0xffffffffffffffff
2022-07-08T00:53:35.616Z **BROKEN** lightningd: FATAL SIGNAL 11 (version c4fe1d3-modded)
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: common/daemon.c:38 (send_backtrace) 0x562b4b73df3e
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: common/daemon.c:46 (crashdump) 0x562b4b73df92
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: /build/glibc-SzIz7B/glibc-2.31/signal/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0 ((null)) 0x7fe0516bc08f
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: ./bitcoin/short_channel_id.h:42 (is_stub_scid) 0x562b4b6b6bb0
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: lightningd/channel.c:789 (channel_fail_permanent) 0x562b4b6b975a
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: lightningd/onchain_control.c:623 (onchaind_funding_spent) 0x562b4b6e4496
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: lightningd/peer_control.c:1638 (funding_spent) 0x562b4b6f9fa9
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: lightningd/watch.c:271 (txowatch_fire) 0x562b4b7142d3
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: lightningd/chaintopology.c:87 (filter_block_txs) 0x562b4b6b3c0d
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: lightningd/chaintopology.c:758 (add_tip) 0x562b4b6b5ac8
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: lightningd/chaintopology.c:850 (get_new_block) 0x562b4b6b6027
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: lightningd/bitcoind.c:388 (getrawblockbyheight_callback) 0x562b4b6b234f
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: lightningd/plugin.c:587 (plugin_response_handle) 0x562b4b7084b1
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: lightningd/plugin.c:693 (plugin_read_json_one) 0x562b4b7086da
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: lightningd/plugin.c:738 (plugin_read_json) 0x562b4b7088be
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: ccan/ccan/io/io.c:59 (next_plan) 0x562b4b7af3ce
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: ccan/ccan/io/io.c:407 (do_plan) 0x562b4b7affd6
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: ccan/ccan/io/io.c:417 (io_ready) 0x562b4b7b0018
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: ccan/ccan/io/poll.c:453 (io_loop) 0x562b4b7b230b
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: lightningd/io_loop_with_timers.c:22 (io_loop_with_timers) 0x562b4b6d6ce4
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: lightningd/lightningd.c:1184 (main) 0x562b4b6dcfc8
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: ../csu/libc-start.c:308 (__libc_start_main) 0x7fe05169d082
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: (null):0 ((null)) 0x562b4b6b138d
2022-07-08T00:53:35.617Z **BROKEN** lightningd: backtrace: (null):0 ((null)) 0xffffffffffffffff

@niftynei
Copy link
Collaborator

test_emergencyrecover is failing under postgres, because it expects a sqlite3 database file to delete


deftest_emergencyrecover(node_factory, bitcoind):
"""
    Test emergencyrecover
    """
        l1, l2 = node_factory.get_nodes(2, opts=[{}, {}])
        l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
        c12, _ = l1.fundchannel(l2, 10**5)
        l1.stop()
>       os.unlink(os.path.join(l1.daemon.lightning_dir, TEST_NETWORK, "lightningd.sqlite3"))
E       FileNotFoundError: [Errno 2] No such file or directory: '/tmp/ltests-a1vrjh39/test_emergencyrecover_1/lightning-1/regtest/lightningd.sqlite3'

@adi2011 adi2011 force-pushed the master branch 2 times, most recently from 98e46fd to 77c4da9 Compare July 13, 2022 01:36
@adi2011
Copy link
Collaborator Author

adi2011 commented Jul 13, 2022

test_emergencyrecover is failing under postgres, because it expects a sqlite3 database file to delete

Solved...

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.

None yet

6 participants