Peer storage feature#5361
Conversation
3627845 to
1589e98
Compare
b4ca14f to
cc0eb71
Compare
vincenzopalazzo
left a comment
There was a problem hiding this comment.
Concept ack I just read the code, it requires more love in the review from my side!
|
Thanks for the review @vincenzopalazzo, I have mostly fixed all the mentioned changes in the SCB PR, This one will get in after that :) |
|
Ops! my bad I did not see that this was built on top of another PR! my bad! |
d790c0a to
9779edc
Compare
niftynei
left a comment
There was a problem hiding this comment.
Half done review, pushing up comments.
In general, smaller commits for each independent change would be great.
plugins/chanbackup.c
Outdated
| } | ||
| }, | ||
| { | ||
| "peerstoragebkp", |
There was a problem hiding this comment.
Commands should always include a verb. Maybe populate-disk-scb-from-peer?
There was a problem hiding this comment.
restorefrompeer looks good 👍, - is not allowed I think, they create problems in other languages.
plugins/chanbackup.c
Outdated
| { | ||
| "peerstoragebkp", | ||
| "recovery", | ||
| "Grabs the scb from datastore (if exists)", |
There was a problem hiding this comment.
Could be more descriptive of what this does?
Checks if i have got a backup from a peer, and if so, will stub those channels in the database and if is successful, will return list of channels that have been successfully stubbed
There was a problem hiding this comment.
This command reads the latest backup storage data from our datastore and populates our database w/ stubs from it.
A few things about this are odd to me.
- How would someone know when to call this command?
- This seems like it could/should happen automatically when we get a scb from a peer?
- How do we know the scb we have in the datastore is valid/up to date? What's the worst case scenario if an out of date scb is applied?
- What happens for channels in the datastore scb that are already in our db?
There was a problem hiding this comment.
Okay, these are some really nice questions:
Ans. 1) This would be used in case of data loss. The user could connect to their peers and collect their storage.
Ans. 2) Hmm, That's an excellent suggestion, everytime we receive scb we can automatically stub the channels in the DB (which are not already into it). What's your opinion on this @rustyrussell ?
Ans. 3) This feature would be used as a last resort to recover the funds in case of severe data loss, This can only help users to recover funds in case of severe data loss, This would never harm existing channels and funds.
Ans. 4) We skip over them and do not stub them in the DB.
plugins/chanbackup.c
Outdated
| after_latestbkp, | ||
| &forward_error, | ||
| NULL); | ||
| json_add_string(req->js, "key", "latestbkp"); |
There was a problem hiding this comment.
nit: call it scb not bkp
plugins/chanbackup.c
Outdated
|
|
||
| } | ||
|
|
||
| static struct command_result *json_peerstoragebkp(struct command *cmd, |
plugins/chanbackup.c
Outdated
| return send_outreq(cmd->plugin, req); | ||
| } | ||
|
|
||
| static struct command_result *after_datastore(struct command *cmd, |
There was a problem hiding this comment.
maybe there's a generic option for htis? if not you can add one.
plugins/chanbackup.c
Outdated
| struct stat st; | ||
| struct node_id *node_id = tal(cmd, struct node_id); | ||
|
|
||
| int fd = open("emergency.recover", O_RDONLY); |
There was a problem hiding this comment.
this really could be a separate function? don't you do this other places?
There was a problem hiding this comment.
filenames in line is not great either fwiw.
There was a problem hiding this comment.
filenames in line is not great either fwiw.
Defined a global var for it.
There was a problem hiding this comment.
this really could be a separate function? don't you do this other places?
Hmm, Done!
| plugin_err(cmd->plugin, "closing: %s", strerror(errno)); | ||
| } | ||
|
|
||
| peers = json_get_member(buf, params, "peers"); |
plugins/chanbackup.c
Outdated
| json_to_node_id(buf, nodeid, node_id); | ||
|
|
||
| req = jsonrpc_request_start(cmd->plugin, | ||
| cmd, |
There was a problem hiding this comment.
I think you're gonna want to make this NULL.
0e11176 to
cc6b450
Compare
cc6b450 to
6f32d6c
Compare
6f32d6c to
fb73a97
Compare
plugins/chanbackup.c
Outdated
| { | ||
| "peerstoragebkp", | ||
| "recovery", | ||
| "Grabs the scb from datastore (if exists)", |
There was a problem hiding this comment.
This command reads the latest backup storage data from our datastore and populates our database w/ stubs from it.
A few things about this are odd to me.
- How would someone know when to call this command?
- This seems like it could/should happen automatically when we get a scb from a peer?
- How do we know the scb we have in the datastore is valid/up to date? What's the worst case scenario if an out of date scb is applied?
- What happens for channels in the datastore scb that are already in our db?
plugins/chanbackup.c
Outdated
|
|
||
| update_scb(cmd->plugin, scb_chan); | ||
| return notification_handled(cmd); | ||
| plugin_log(cmd->plugin, LOG_INFORM, "Updating the SCB2"); |
There was a problem hiding this comment.
what's an "SCB2"? maybe make LOG_DBG
There was a problem hiding this comment.
Removed, Was using it for debugging
plugins/chanbackup.c
Outdated
| tal_bytelen(serialise_scb)); | ||
|
|
||
| send_outreq(cmd->plugin, req); | ||
| } |
There was a problem hiding this comment.
what happens if not connected?
plugins/chanbackup.c
Outdated
|
|
||
| struct info { | ||
| size_t idx; | ||
| const char *buf; |
plugins/chanbackup.c
Outdated
| { | ||
| plugin_log(cmd->plugin, LOG_INFORM, "Peer storage sent to"); | ||
| info->idx += 1; | ||
| return after_listpeers(cmd, info->buf, json_parse_simple(cmd, info->buf, tal_bytelen(info->buf)), info); |
There was a problem hiding this comment.
i would build a list of connected peers and iterate thru that, rather than holding onto the buffer w/ all the data in memory for the duration of this.
There was a problem hiding this comment.
Done, we just need the index. Libplugin automatically handles multiple requests one by one
fb73a97 to
4121d56
Compare
|
@adi2011: this is marked for release 22.11 for which we're trying to publish a first RC this week, but it is also marked as a draft. What's the current status of this? Happy to review and merge it if it is ready, otherwise we can also postpone it to the next release, which'll happen in ~2 months again. |
|
After talking to @adi2011 we decided that we'll be pushing this PR to the next release. |
|
Thanks! Will get onto it once my semester exams are over. |
4121d56 to
b0d028a
Compare
…peer_connected. This is needed for the next patch, which does this from the peer_connected hook! Signed-off-by: Rusty Russell <[email protected]> Changelog-Changed: JSON-RPC: `sendcustommsg` can now be called by a plugin from within the `peer_connected` hook.
Add msg type peer_storage and your_peer_storage
We are now going to have messages which we know about, but yet we don't handle ourselves. [ I reversed this from Adi's, as that was clearer! --RR ]
Signed-off-by: Rusty Russell <[email protected]>
And we should always represent them as is, not as optional: it's possible in future we could *require* "WANT_PEER_BACKUP_STORAGE". Signed-off-by: Rusty Russell <[email protected]>
node_id can be on the stack, avoiding a tal call. Signed-off-by: Rusty Russell <[email protected]>
ab73723 to
5805664
Compare
|
Dumb typo completely broke peer_connected hook! Ack 5805664 |
When you return an allocated pointer, you should always hand in the context you want it allocated from. This is more explicit, because it may really matter to the caller! This also folds some simple operations, and avoids doing too much variable assignment in the declarations themselves: some coding styles prohibit such initializers, but that's a bit exteme. Signed-off-by: Rusty Russell <[email protected]>
Since it's not spec-final yet (hell, it's not even properly specified yet!) we need to put it behind an experimental flag. Unfortunately, we don't have support for doing this in a plugin; a plugin must present features before parsing options. So we need to do it in core. Signed-off-by: Rusty Russell <[email protected]>
5805664 to
081a1d8
Compare
|
Ack 081a1d8 |
|
Congrats @adi2011! It was a long road, but this is a very cool feature! |
|
Thanks and congratulations to you too! :) |
|
|
Is there any documentation for this, other than the name of the option? In particular it would be useful to illustrate how recovery works, e.g. "Start a new node with the same secret, find one of your old peers e.g. using a public archive like mempool . space, call lightning-cli restore-from-peer, etc".
|
PEER STORAGE BACKUP
This PR implements
peer storage backupwhich will enable nodes to exchange their respective SCBs (Static channel backup), This will be useful in case of complete data loss.WHY
One of the major features of lightning is that the transactions are off-chain and are stored in a local database, which makes this DB highly dynamic and complex to backup.
Peer storage backup will allow users to send their encrypted backup to their peers, which can be used to recover their funds in case of complete data loss.
HOW
The strategy is that we'll store the data received in the datastore.
So to implement this I've introduced 2 new messages i.e.
PEER_STORAGEandYOUR_PEER_STORAGE, these will be used to exchange the data stored between the nodes.-
PEER_STORAGEwill be used to send our own encrypted backup to the peer.-
YOUR_PEER_STORAGEwill be used to send the most recent backup of the peer.The general flow of messages every time we
connectis:Alice -----------------------♥️ ------------------------------ Bob
PEER_STORAGE 📤-------------📨-------------> 📥
YOUR_PEER_STORAGE 📤---------📨------------> 📥
📥 <------------📨--------------PEER_STORAGE 📤
📥 <-----------📨------------YOUR_PEER_STORAGE 📤
On receiving
YOUR_PEER_STORAGEbob will verify if it's correct and Alice has not changed anything in his last sent backup.On receiving
PEER_STORAGEbob will update the backup he has stored for Alice in his own datastore.Every time we open a new channel or close an old one we will send
PEER_STORAGEto every peer we're connected to (Haven't figured out yet, maybe a new RPC (sendcustommsgmulti) which will enable plugins to send a single message to multiple users at once, because the interaction between lightningd and plugins is single-threaded)They can choose to ignore the messages since they are odd (
It's okay to be odd)In case of complete data loss, the user will reconnect to their peers and hope that they get a YOUR_PEER_STORAGE message. Then they can directly use the RPC specified in the plugin to recover the channels.