Skip to content

Add onchain fund recovery test #396

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

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

tnull
Copy link
Collaborator

@tnull tnull commented Nov 4, 2024

We add a test we recover on-chain funds from a given seed after the initial sync.

This extends the test coverage that would have caught #383.

@tnull tnull requested a review from jkczyz November 4, 2024 09:21

let chain_source = TestChainSource::Esplora(&electrsd);

let seed_bytes: Vec<u8> = std::iter::repeat(42u8).take(64).collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: vec![42u8; 64]

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, added a fixup.

@@ -302,6 +304,11 @@ pub(crate) fn setup_node(chain_source: &TestChainSource, config: Config) -> Test
builder.set_chain_source_bitcoind_rpc(rpc_host, rpc_port, rpc_user, rpc_password);
},
}

if let Some(seed) = seed_bytes {
builder.set_entropy_seed_bytes(seed).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this PR, but why don't we take [u8; WALLET_KEYS_SEED_LEN]? Bindings?

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, bindings unfortunately can't handle raw arrays. We could eventually consider exposing an alternative method based on the uniffi feature.

original_node.sync_wallets().unwrap();
assert_eq!(
original_node.list_balances().spendable_onchain_balance_sats,
premine_amount_sat * 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't quite follow this. Is the above a self-send? Or is there a second wallet somewhere that isn't immediately obvious to me?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can't quite follow this. Is the above a self-send? Or is there a second wallet somewhere that isn't immediately obvious to me?

There indeed is a second wallet, namely the one in our bitcoind instance. As all mining rewards accrue there, we distribute funds by sending them from this wallet to the LDK Node wallet via the send_to_address RPC command.

@tnull
Copy link
Collaborator Author

tnull commented Nov 4, 2024

CI failure is unrelated (MSRV violation to be fixed by #398)

@tnull tnull force-pushed the 2024-11-onchain-recovery-test branch from 38c60de to 31ec815 Compare November 4, 2024 19:18
@tnull
Copy link
Collaborator Author

tnull commented Nov 4, 2024

CI failure is unrelated (MSRV violation to be fixed by #398)

Rebased after #398 landed to fix CI.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM. Please squash.

We add a test that makes sure a completely new node is able to recover
any previously-confirmed on-chain funds from the seed only.
@tnull tnull force-pushed the 2024-11-onchain-recovery-test branch from 31ec815 to c17c995 Compare November 5, 2024 08:58
@tnull
Copy link
Collaborator Author

tnull commented Nov 5, 2024

Squashed commits without further changes. CI failure is yet another MSRV violation (i.e. unrelated).

@tnull tnull merged commit 8f61e46 into lightningdevkit:main Nov 5, 2024
4 of 13 checks passed
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.

2 participants