Repository navigation
Conversation
|
👋 Thanks for assigning @tankyleo as a reviewer! |
Our wallet needs per-input finalization results to distinguish unfinished foreign inputs from failures to finalize its own inputs. Raise the minimum BDK Wallet version to 3.2, which provides these results alongside fixes for TRUC input selection and changeset merging. Co-Authored-By: HAL 9000
BDK's wallet-owned signing APIs are deprecated, but our signing paths still need their witness-UTXO policy, sighash checks, low-R signatures, and input finalization. Use caller-owned signers for both keychains to preserve these signing options while dropping the deprecated APIs. Keep signing available after reload, and cover witness-only signing from both keychains. Refs lightningdevkit#654 Co-Authored-By: HAL 9000
Concurrent requests sharing an Electrum connection can remain blocked when the active reader sees a clean EOF. This can keep blocking Tokio workers alive through node shutdown. Require the released fix in electrum-client 0.25.1 and cover the failure at the LDK Node boundary so dependency updates cannot reintroduce the hang. BDK Electrum now supports this client version. Fixes lightningdevkit#1007 Refs lightningdevkit#1055 (cherry picked from commit aff6ce0) Co-Authored-By: HAL 9000
Mixed wallet and LDK transactions could be returned with unsigned wallet inputs because the aggregate signing result also accounts for unfinished foreign inputs. Require BDK to finalize every recognized wallet input while allowing inputs without a wallet descriptor to remain unfinished. Cover both signing paths with tests that exercise missing wallet signers. Co-Authored-By: HAL 9000
The maximum funding estimate relies on BDK's input selection. The BDK 3.2 TRUC filtering change needs coverage at this boundary. Check that unconfirmed v2 change remains usable, while v3 outputs only increase the available funding amount after confirmation. Co-Authored-By: HAL 9000
2acfbd1 to
32f0c52
Compare
tankyleo
left a comment
tankyleo
left a comment
There was a problem hiding this comment.
I also note that we currently include unconfirmed outputs from TRUC transactions in BalanceDetails::spendable_onchain_balance_sats, even though our onchain wallet currently never spends them. I think this is OK since we don't expect our onchain funds to be transaction outputs of V3 transactions.
| assert_eq!(tx.input[0].witness.len(), 2); | ||
| assert!(tx.input[1].witness.is_empty(), "foreign inputs may remain unsigned"); | ||
|
|
||
| Arc::get_mut(&mut wallet).unwrap().signers = Default::default(); |
There was a problem hiding this comment.
Can we also test that if we are able to sign for one input using say the external signer, but are missing the internal signer for another input, that we fail signing ?
| for (index, input_outcome) in outcome.into_outcomes() { | ||
| // MissingDescriptor is expected for LDK's inputs, which BDK cannot finalize. | ||
| // A descriptor that BDK recognizes must be satisfied before we return the transaction. | ||
| if let FinalizeInputOutcome::CouldNotSatisfy(e) = input_outcome { |
There was a problem hiding this comment.
I'd prefer to match on every outcome variant here so that if BDK introduces a new variant in the future we are forced to assess whether we should ignore or error for that variant.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn sign_both_keychains_before_and_after_reload() { |
There was a problem hiding this comment.
Can we also expand do_persistence_backwards_compatibility in tests/integration_tests_rust.rs to make sure that signing for both keychains works across an upgrade to this upcoming release ?
| thread::sleep(Duration::from_millis(50)); | ||
| } | ||
| requests_received | ||
| }); |
There was a problem hiding this comment.
Here we close the entire socket while request data may still be unread. This can send a TCP reset, which the old client already handles correctly, allowing the test to pass without exercising the EOF bug.
We should shut down only the server’s sending side first, then read the remaining requests before fully closing the socket.
| let mut request = String::new(); | ||
| match reader.read_line(&mut request) { | ||
| Ok(0) => break, | ||
| Ok(_) => requests_received += 1, |
There was a problem hiding this comment.
Let’s also ensure multiple requests are fully sent before shutting down the server’s sending side. Otherwise, the remaining requests could fail while writing, allowing the test to pass without verifying that EOF wakes multiple requests waiting for responses.
3 participants
Closes #654
Closes #1055
We bump
bdk_walletdependency, migrate to the updated signing API, and finally add some regression tests for the Electrum EOF fix that shipped inelectrum-client.