Add bitcoind RPC support #370

tnull · 2024-10-10T15:37:19Z

Based on #365.

We add support for sourcing chain data from the bitcoind RPC interface. Ready for review.

TODO:

Figure out how to deal fully with estimatesmartfee in tests. Probably just do a simliar carve-out for non-mainnet as for Esplora, i.e., default to 253?
Figure out how to deal with explicit sync_wallet calls. No-op? Or still block until the next polling is finished?
Depends on lightning-block-sync: Fix synchronize_listeners always calling default implementation rust-lightning#3354, so we might temporarily need to copy/past synchronize_listeners over here.

~~- [ ] Implement UtxoSource / GossipVerifier (via tokio feature of lightning-block-sync)~~ (Bashing my head against the typechecker for a few hours is having me believe this is currently ~impossible with the upstream types, so moved to #377 / blocked on lightningdevkit/rust-lightning#3369)

tnull · 2024-10-15T08:04:04Z

Now rebased on #365 after we bumped to 0.0.125, i.e., also dropped the commit copy/pasting synchronize_listeners as the fix was part of the release.

tnull · 2024-10-15T15:15:09Z

Alright, rebased on main after #365 landed. Also addressed (or postponed) the remaining TODOs, should be ready for review.

src/chain/bitcoind_rpc.rs

src/chain/mod.rs

jkczyz · 2024-10-15T22:10:16Z

tests/integration_tests_rust.rs

+#[test]
+fn channel_full_cycle_bitcoind() {
+	let (bitcoind, electrsd) = setup_bitcoind_and_electrsd();
+	premine_dummy_blocks(&bitcoind.client, &electrsd.client);
+	let chain_source = TestChainSource::BitcoinRpc(&bitcoind);
+	let (node_a, node_b) = setup_two_nodes(&chain_source, false, true, false);
 	do_channel_full_cycle(node_a, node_b, &bitcoind.client, &electrsd.client, false, true, false);
 }


Why not expand the other tests for BitcoinRpc?

Generally sounds good, I mostly started off with this as I didn't simply want to double our test surface. Do you have any specific test cases in mind that would lend themselves to also be run on a BitcoindRpc backend?

Hmm... probably only tests where non-setup code depends on block (dis)connection notification (e.g., onchain_spend_receive). Though, I also wonder if we should use BitcoinRpc normally just to remove one moving part from the tests assuming that would lead to less flakiness.

Though, I also wonder if we should use BitcoinRpc normally just to remove one moving part from the tests assuming that would lead to less flakiness.

I generally agree this would be preferable but thought we might be limited by the additional overhead of the premining required to populate estimatesmartfee. As we now added fallbacks for regtest anyways we can however drop that premining again.

I now added a fixup switching it around: we now default to bitcoind RPC and only have a single Esplora-specific full-cycle test. It seemed a bit flaky when run concurrently locally, but let's see how it works out in CI. If it turns out to be flaky we can take additional mitigation steps in a follow up (e.g., reduce the number of threads used in tests, etc).

Right, if the extra part is actually removing the underlying flakiness, maybe we should keep it. 😛 Though perhaps we need to make sure the (non-test) code is robust enough to retry appropriately.

Ugh, there we have the first CI failure due to flakiness. Mind if I revert and do the CI switch as a follow-up (possibly post-release)? I'd like to get main into a release-ready state ASAP.

Now reverted, but still dropped the unnecessary premining.

jkczyz

LGTM modulo test changes

jkczyz · 2024-10-16T16:08:00Z

tests/integration_tests_rust.rs

+#[test]
+fn channel_full_cycle_bitcoind() {
+	let (bitcoind, electrsd) = setup_bitcoind_and_electrsd();
+	premine_dummy_blocks(&bitcoind.client, &electrsd.client);
+	let chain_source = TestChainSource::BitcoinRpc(&bitcoind);
+	let (node_a, node_b) = setup_two_nodes(&chain_source, false, true, false);
 	do_channel_full_cycle(node_a, node_b, &bitcoind.client, &electrsd.client, false, true, false);
 }


Hmm... probably only tests where non-setup code depends on block (dis)connection notification (e.g., onchain_spend_receive). Though, I also wonder if we should use BitcoinRpc normally just to remove one moving part from the tests assuming that would lead to less flakiness.

.. which we'll use to feed blocks to it in following commits.

tnull · 2024-10-16T16:48:01Z

LGTM modulo test changes

Cool, let me know if I can squash.

jkczyz

LGTM, please squash

We first initialize by synchronizing all `Listen` implementations, and then head into a loop continuously polling our RPC `BlockSource`. We also implement a `BoundedHeaderCache` to limit in-memory footprint.

.. to allow the on-chain wallet to detect what's inflight.

This behavior mirrors what we do in the Esplora case: we only enforce successful fee rate updates on mainnet. On regtest/signet/testnet we will just skip (i.e. return `Ok(())`) if we fail to retrieve the updates (e.g., when bitcoind's `estimatesmartfee` isn't sufficiently populated) and will either keep previously-retrieved values or worst case fallback to the fallback defaults.

.. as it regularly makes CI fail and doesn't provide us anything really.

.. to account for slight differences in fee rate estmations between chain sources.

tnull · 2024-10-16T17:55:57Z

Squashed fixups without further changes.

enigbe · 2024-10-17T18:55:13Z

src/chain/mod.rs

+					(sweeper_best_block_hash, &*output_sweeper as &(dyn Listen + Send + Sync)),
+				];
+
+				// TODO: Eventually we might want to see if we can synchronize `ChannelMonitor`s


Apologies for the question post-merge but what would be the benefit of syncing the ChannelMonitors before passing them to the ChainMonitor? There would still be a need to sync them via the monitor even if it was possible to sync before, right?

post-merge questions are great!

ChannelMonitors can by sync'd by just syncing the ChainMonitor and it'll multiplex the events across all the ChannelMonitors for us (cleaning up the logic here quite a bit). Sadly, doing so requires that the ChannelMonitors be in sync with each other before we pass them to ChainMonitor on load (or we'll end up connecting blocks on different ChannelMonitors even though they have different ideas of the current chain tip).

tnull marked this pull request as draft October 10, 2024 15:37

This was referenced Oct 11, 2024

Ldk-Server Milestone 2 lightningdevkit/ldk-server#10

Open

Upgrade to LDK 0.0.125 lightningdevkit/ldk-sample#136

Merged

tnull added this to the 0.4 milestone Oct 14, 2024

tnull force-pushed the 2024-10-add-bitcoind-support branch 2 times, most recently from a2b10ef to d9cc69a Compare October 15, 2024 08:03

tnull mentioned this pull request Oct 15, 2024

LDK Node v0.4 Tracking Issue #167

Closed

8 tasks

tnull force-pushed the 2024-10-add-bitcoind-support branch from d9cc69a to 6e6e9b7 Compare October 15, 2024 15:13

tnull changed the title ~~WIP Add bitcoind RPC support~~ Add bitcoind RPC support Oct 15, 2024

tnull marked this pull request as ready for review October 15, 2024 15:14

tnull requested a review from jkczyz October 15, 2024 15:15

tnull force-pushed the 2024-10-add-bitcoind-support branch 3 times, most recently from f68adfc to 7c1ad82 Compare October 15, 2024 17:26

jkczyz reviewed Oct 15, 2024

View reviewed changes

tnull force-pushed the 2024-10-add-bitcoind-support branch 3 times, most recently from 83b0a4f to c20359a Compare October 16, 2024 08:24

jkczyz reviewed Oct 16, 2024

View reviewed changes

tnull added 2 commits October 16, 2024 18:37

Implement Listen for Wallet

0bec579

.. which we'll use to feed blocks to it in following commits.

Add ChainSource::BitcoindRpc variant and basic client struct

c59d781

tnull force-pushed the 2024-10-add-bitcoind-support branch from c20359a to a45c7a7 Compare October 16, 2024 16:41

tnull force-pushed the 2024-10-add-bitcoind-support branch 3 times, most recently from 4839e10 to f4538a3 Compare October 16, 2024 17:21

jkczyz reviewed Oct 16, 2024

View reviewed changes

Add initial polling logic

ab5a620

We first initialize by synchronizing all `Listen` implementations, and then head into a loop continuously polling our RPC `BlockSource`. We also implement a `BoundedHeaderCache` to limit in-memory footprint.

tnull added 10 commits October 16, 2024 19:55

Add comments clarifying that syncing methods shouldn't be used for RPC

b756d79

Implement broadcast queue processing

935a17e

Implement fee rate estmation updating

ce3606e

Retrieve and apply unconfirmed transactions from the mempool

8e1dda1

.. to allow the on-chain wallet to detect what's inflight.

Allow to configure bitcoind RPC in Builder

6da0b98

Add ChainSource support to tests

0a65272

Drop flaky connect_to_public_esplora test

4a086c3

.. as it regularly makes CI fail and doesn't provide us anything really.

Relax onchain_fee_buffer in tests slightly

f165c74

.. to account for slight differences in fee rate estmations between chain sources.

Update README to reflect bitcoind RPC support

7c629f2

tnull force-pushed the 2024-10-add-bitcoind-support branch from f4538a3 to 7c629f2 Compare October 16, 2024 17:55

jkczyz approved these changes Oct 16, 2024

View reviewed changes

tnull merged commit af5d85a into lightningdevkit:main Oct 16, 2024
6 of 13 checks passed

enigbe reviewed Oct 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bitcoind RPC support #370

Add bitcoind RPC support #370

tnull commented Oct 10, 2024 •

edited

Loading

tnull commented Oct 15, 2024

tnull commented Oct 15, 2024

jkczyz Oct 15, 2024

tnull Oct 16, 2024

jkczyz Oct 16, 2024

tnull Oct 16, 2024 •

edited

Loading

jkczyz Oct 16, 2024

tnull Oct 16, 2024

tnull Oct 16, 2024

jkczyz left a comment

jkczyz Oct 16, 2024

tnull commented Oct 16, 2024

jkczyz left a comment

tnull commented Oct 16, 2024

enigbe Oct 17, 2024

TheBlueMatt Oct 17, 2024

Add bitcoind RPC support #370

Add bitcoind RPC support #370

Conversation

tnull commented Oct 10, 2024 • edited Loading

tnull commented Oct 15, 2024

tnull commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Oct 16, 2024

jkczyz left a comment

Choose a reason for hiding this comment

tnull commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Oct 10, 2024 •

edited

Loading

tnull Oct 16, 2024 •

edited

Loading