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

Bitcoin backend generalization #3488

Merged
merged 21 commits into from Feb 12, 2020

Conversation

darosior
Copy link
Collaborator

@darosior darosior commented Feb 6, 2020

This standardizes our requests for Bitcoin data as a set of commands plugins can register, and makes our bitcoind interaction through bitcoin-cli the default plugin, which registers all of these commands.

This makes it possible to not rely on a bitcoin-core backend, which can help for instance for mobile integration (cf #3484).

See #3354 for some design decisions, but here is a summary:

  • Separated plugins can register different calls, which allows plugins specialization (credits to @cdecker)
  • Uses JSON-RPC commands instead of hooks (see #3354).
  • Needs 5 commands to be registered by our plugin(s):
    • getchaininfo - called at startup to get infos about our network, and about the block chain.
    • getrawblockatheight - get the block at that height in hex format, along with its id.
    • sendrawtransaction - self explanatory
    • gettxout - get the amount and the scriptPubkey of this txo.
    • getfeerate - get the feerate in btc/kVB, because we love vbytes for fee-sensitive Bitcoin applications. Params are bitcoind-style.

This is based on #3480 as it uses its higher-level JSONRPC helpers for plugins.

TODO

  • bcli-specific functional tests
  • interface documentation, for helping Bitcoin L1 developers to develop a plugin to serve as a C-lightning Bitcoin backend :)

Next

  • Add the possibility for the Bitcoin plugin to send notifications to lightningd (like "got a new block")
  • Add a Bitcoin backend plugin for [insert Bitcoin data source]

@darosior
Copy link
Collaborator Author

@darosior darosior commented Feb 6, 2020

A minimal plugin which uses an esplora instance instead of bitcoind to serve Bitcoin data to C-lightning can be found here lightningd/plugins#89.

@darosior darosior force-pushed the bitcoin_backend_plugin branch 3 times, most recently from 1eb5490 to 3cf1571 Compare Feb 6, 2020
@ZmnSCPxj
Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj commented Feb 7, 2020

Spurious failure:

 lightningd/lightningd: --bitcoin-rpcuser=rpcuser: unrecognized option

https://travis-ci.org/ElementsProject/lightning/jobs/647097656#L1083

That seems a strange error to report.

@darosior
Copy link
Collaborator Author

@darosior darosior commented Feb 7, 2020

That seems a strange error to report

This is just that I remove the default plugin in one of the tests, to test the Bitcoin plugin registration behaviour.
But I'll give it a look anyway.
EDIT: Ok, looks like my hacky removal of bcli doesnt clean parallelize well .. Ok seems better to use the dedicated option..

@darosior darosior force-pushed the bitcoin_backend_plugin branch from 3cf1571 to 083d123 Compare Feb 7, 2020
@darosior
Copy link
Collaborator Author

@darosior darosior commented Feb 7, 2020

Still one unrelated spurious Travis error, and two jobs timeout..

@darosior darosior force-pushed the bitcoin_backend_plugin branch from 083d123 to 8fe0233 Compare Feb 8, 2020
@darosior
Copy link
Collaborator Author

@darosior darosior commented Feb 8, 2020

I've removed the doc from the TODOs after discussion with @cdecker on IRC so that it doesn't lock up with that interface too early. I had already written it so if reviewers find it useful, you can find it here.

@darosior darosior marked this pull request as ready for review Feb 8, 2020
@darosior darosior requested a review from cdecker as a code owner Feb 8, 2020
@rustyrussell rustyrussell added this to the 0.8.1 milestone Feb 10, 2020
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Only one significant change req, really: split rpc-file creation from rpc servicing, and do creation before starting plugins.

But also needs:

  1. New section in doc/PLUGINS.md for these interfaces.
  2. Changelog bragging line! :)

plugins/libplugin.h Show resolved Hide resolved
lightningd/bitcoind.c Outdated Show resolved Hide resolved
lightningd/bitcoind.c Show resolved Hide resolved
lightningd/bitcoind.c Show resolved Hide resolved
lightningd/bitcoind.c Show resolved Hide resolved
lightningd/bitcoind.c Outdated Show resolved Hide resolved
@darosior
Copy link
Collaborator Author

@darosior darosior commented Feb 10, 2020

Only one significant change req, really: split rpc-file creation from rpc servicing, and do creation before starting plugins.

Ok so I tempted but there is a circular dependency which causes a deadlock.
If we want the JSONRPC interface to be set up before topology but not serve requests until topology is setup, we can simply:

diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c
index d2f896904..15a2df66a 100644
--- a/lightningd/jsonrpc.c
+++ b/lightningd/jsonrpc.c
@@ -984,6 +984,13 @@ static struct io_plan *jcon_connected(struct io_conn *conn,
 static struct io_plan *incoming_jcon_connected(struct io_conn *conn,
                                               struct lightningd *ld)
 {
+       /* The topology is required to be ready by most of the JSONRPC calls,
+        * even the simplest ones (`getinfo` queries the tip). However we
+        * want to serve the RPC socket before we setup the topology (mostly
+        * for plugins), so accept incoming requests but don't treat them yet. */
+       if (!ld->topology->ready)
+               return io_wait(conn, ld->jsonrpc, incoming_jcon_connected, ld);
+
        /* Lifetime of JSON conn is limited to fd connect time. */
        return jcon_connected(notleak(conn), ld);
 }
diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c
index 0874bee54..594383fff 100644
--- a/lightningd/lightningd.c
+++ b/lightningd/lightningd.c
@@ -855,6 +855,11 @@ int main(int argc, char *argv[])
        setup_topology(ld->topology, ld->timers,
                       min_blockheight, max_blockheight);
 
+       /* Now that the topology is set up, we can treat incoming JSONRPC
+        * requests ! */
+       assert(ld->topology->ready);
+       io_wake(ld->jsonrpc);
+

But we need to start plugins in order to setup the topology.... And plugins use libplugin which requires the JSONRPC to answer a request (listconfigs in order to set the allow_deprecated_apis global) before responding to init.

So either we get rid of this call in handle_init, which I believe is not acceptable, or we setup the plugins needed to setup the topology before the RPC socket (and other current plugins), or we special-case the bcli plugin to not require this call to listconfigs, and synchronously wait for it to be configured in setup_topology().. Which is about the same situation as now but in a less explicit way.

I think the right thing would be to make a distinction between plugins which rely upon lightningd (and thus need the RPC socket, are setup after the topology, etc..), and plugins which lightningd relies upon.
That said, with this PR the only plugin(s) lightningd relies on are bitcoind-ones, so it's special-cased into lightningd/bitcoind(and this is cleaner than all my attempts to create the socket before starting these plugins).

@darosior darosior force-pushed the bitcoin_backend_plugin branch from 8fe0233 to 07dc09f Compare Feb 10, 2020
@darosior
Copy link
Collaborator Author

@darosior darosior commented Feb 10, 2020

Rebased, renamed gettxout to getutxout, removed the requirement for errmsg to be present if sendrawtransaction succeeds, and generalized find_plugin_for_command().

The doc is ready but I wanted @cdecker feedback before as he had a reserve about documenting too early.

@darosior darosior force-pushed the bitcoin_backend_plugin branch 2 times, most recently from 35e7bc8 to 3b463f9 Compare Feb 10, 2020
@rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Feb 11, 2020

Yeah, OK, let's do this for now then. Nasty stuff though :(

@rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Feb 11, 2020

Trivial rebase. Still needs a Changelog entry though!

@darosior darosior force-pushed the bitcoin_backend_plugin branch from 8d8fdee to f1d4322 Compare Feb 11, 2020
darosior added 7 commits Feb 11, 2020
We don't take the callback result into account, so it can better be void.
Having a general callback parameter is handy, because for bcli we want
to pass it the struct bcli.
Most is taken from lightningd/bitcoind and adapted. This currently
exposes 5 commands:
- `getchaininfo`, currently called at startup to check the network and
  whether we are on IBD.
- `getrawblockbyheight`, which basically does the `getblockhash` +
  `getblock` trick.
- `getfeerate`
- `sendrawtransaction`
- `getutxout`, used to gather infos about an output and currently used by
  `getfilteredblock` in `lightningd/bitcoind`.
This is also taken and adapted from lightningd/bitcoind.

The call to 'getblockchaininfo' is replaced by 'echo' as we don't
make use of the result and the former can sometimes be slow (e.g. on
IBD).
We are going to initialize a plugin before its creation, so log as
UNUSUAL instead.

Also, `pay` and `fundchannel` inits are using rpc_delve(), so we need to
io_new_conn() (which sets the socket as non blocking) after calling the
plugin's init.
Exit early if we won't be able to fully communicate with our Bitcoin
backend.
darosior added 12 commits Feb 11, 2020
We need our Bitcoin backend to be initialized, but the plugins have not yet been
started at this point.
The Bitcoin backend is generalized through the Bitcoin plugin and this
was specific to core.
This adds `getchaininfo` and `getrawblockbyheight` handling lightningd-side,
and use them in setup_topology().

We then remove legacy bitcoind_getblockcount() (we already get the count in
`getchaininfo`), bitcoind_getblockchaininfo() (it was only used in setup_topology()),
and wait_for_bitcoind() (this was specific to bitcoin-core and we assume our Bitcoin
backend to be functional if the plugin responds to `init`).
This restrains the informations we get about how the sending went to
an errmsg as we cant rely on bitcoin-cli specific output nor its exit code.
This avoids the getblockhash+getblock, and more importantly that was the
last functionality making use of bitcoind_getrawblock() and bitcoin_getblockhash(),
so we can also get rid of them.
And remove bitcoin-cli interaction code, now unused.
Changelog-Added: pluggable backends for Bitcoin data queries, default still bitcoind (using bitcoin-cli).
We need our Bitcoin backend to be ready to get infos about some utxos
For bitcoind_fail_first:
We only ever send `getblock` if we got a successful block hash from
`getblockhash`, and if we can't get the block in that case it means
our Bitcoin backend is faulty and we shouldnt continue.

So, mock `getblockhash` instead, which is authorized to spuriously fail.

For both bitcoind_fail_first and bitcoind_failure:
Adapt the logs.
@darosior darosior force-pushed the bitcoin_backend_plugin branch from f1d4322 to 95563a7 Compare Feb 11, 2020
@darosior
Copy link
Collaborator Author

@darosior darosior commented Feb 11, 2020

Rebased and changeloged

Copy link
Member

@cdecker cdecker left a comment

Still running some more tests (especially against TEST_DB_PROVIDER=postgres and TEST_NETWORK=liquid-regtest) to make sure this is stable in all configurations. I'll add fixups if necessary.

plugins/libplugin.h Show resolved Hide resolved
plugins/libplugin.h Show resolved Hide resolved
plugins/bcli.c Show resolved Hide resolved
db_begin_transaction(call->bitcoind->ld->wallet->db);
call->cb(call->bitcoind, NULL, NULL, call->cb_arg);
db_commit_transaction(call->bitcoind->ld->wallet->db);
Copy link
Member

@cdecker cdecker Feb 11, 2020

Choose a reason for hiding this comment

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

Shouldn't this already be wrapped in a DB transaction by virtue of being part of the io_loop?

Copy link
Collaborator Author

@darosior darosior Feb 11, 2020

Choose a reason for hiding this comment

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

I don't see why it should: lightningd/io_loop_with_timers only begins a db_transaction is a timer expires (out of the io_loop).

Before this bcli_finished would make a db transaction for each callback

db_begin_transaction(bitcoind->ld->wallet->db);
ok = bcli->process(bcli);
db_commit_transaction(bitcoind->ld->wallet->db);

So process_getblock was also inside a db_transaction

db_begin_transaction(call->bitcoind->ld->wallet->db);
call->cb(call->bitcoind, &blkid, blk, call->cb_arg);
db_commit_transaction(call->bitcoind->ld->wallet->db);
Copy link
Member

@cdecker cdecker Feb 11, 2020

Choose a reason for hiding this comment

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

Same here

lightningd/chaintopology.c Show resolved Hide resolved
struct jsonrpc_request *req;

req = jsonrpc_request_start(bitcoind, "getfeerate",
bitcoind->log, getfeerate_callback,
call);
json_add_num(req->stream, "blocks", call->blocks[call->i]);
json_add_string(req->stream, "mode", call->estmode[call->i]);
jsonrpc_request_end(req);
plugin_request_send(strmap_get(&bitcoind->pluginsmap,
"getfeerate"), req);
Copy link
Member

@cdecker cdecker Feb 11, 2020

Choose a reason for hiding this comment

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

Would be nice if we could bundle these and send all the block/mode combinations to the plugin at once. That'd allow us to have a single all-or-nothing handler and not having to loop in lightningd.

Copy link
Collaborator Author

@darosior darosior Feb 11, 2020

Choose a reason for hiding this comment

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

Indeed. As a cleanup or before the release ?

Copy link
Contributor

@rustyrussell rustyrussell Feb 11, 2020

Choose a reason for hiding this comment

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

Post. Too late for release now.

Copy link
Collaborator Author

@darosior darosior Feb 12, 2020

Choose a reason for hiding this comment

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

Opened #3508 to track this.

lightningd/bitcoind.c Show resolved Hide resolved
tests/test_plugin.py Show resolved Hide resolved
tests/test_plugin.py Outdated Show resolved Hide resolved
@darosior
Copy link
Collaborator Author

@darosior darosior commented Feb 11, 2020

Spurious Travis error

@cdecker
Copy link
Member

@cdecker cdecker commented Feb 11, 2020

Let's see if @bitcoin-bot can recogniZe clean squashes yet 😉

ACK 93596e3

@rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Feb 11, 2020

Apparently not :( Mind you, GH doesn't give anything for that 'force-push' link either...

@rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Feb 12, 2020

Squashed fixup:

Ack f7b48ed

@cdecker
Copy link
Member

@cdecker cdecker commented Feb 12, 2020

Interestingly the web frontend seems to recognize them as identical (green background for the comparison between the last two versions). I'll look into it

@rustyrussell rustyrussell merged commit a47fd8c into ElementsProject:master Feb 12, 2020
4 checks passed
@darosior darosior deleted the bitcoin_backend_plugin branch Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bitcoind Optech Make Me Famous!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants