Follow-ups to #660#992
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
| use crate::Error; | ||
|
|
||
| const BCAST_PACKAGE_QUEUE_SIZE: usize = 50; | ||
| const BCAST_PACKAGE_QUEUE_SIZE: usize = 256; |
There was a problem hiding this comment.
I went back and forth with codex on this one, for now I lean against drawing transactions at random order: we do not do this now, but in the future we might want to make sure we broadcast parents before children no ?
There was a problem hiding this comment.
but in the future we might want to make sure we broadcast parents before children no ?
Well, in short, I'm not sure we could even begin to guarantee this? Broadcast is inherently fallible, we never know when the network connection could drop, when the backend won't accept anything into the mempool, and when it will just decide to drop any transaction again. So without mempool introspection I'd always lean on treating broadcast as an entirely opaque operation: we submit and retry, and only stop once we see what we expect confirmed in a block.
There was a problem hiding this comment.
sounds good added the random draw below
| } | ||
|
|
||
| fn esplora_submitpackage_error_implies_unsupported(e: &esplora_client::Error) -> bool { | ||
| matches!(e, esplora_client::Error::HttpResponse { status: 400 | 404, .. }) |
There was a problem hiding this comment.
We map 400 here to ChainSourceNotSupported because Bitcoin Core v26 returns a RPC error when submitting the dummy package, which maps to error code 400.
There was a problem hiding this comment.
Grrr, this is all very brittle. I can't wait to drop all of this logic again once we can just assume submitpackage is available if we have any chain source at all.
| })?; | ||
| }); | ||
| if let Err(e) = startup_chain_check_res { | ||
| self.chain_source.stop(); |
There was a problem hiding this comment.
Actually, it seems there are a bunch of (pre-existing) potential cases below where we'd error. To solve this once and for all, let's rename start to start_inner and add a new pub fn start that wraps that start_inner (probably also takes the running lock) and stops the chain source for any error returned.
| - name: Test with 0FC enabled | ||
| run: | | ||
| RUSTFLAGS="--cfg no_download --cfg cycle_tests --cfg tokio_unstable --cfg zero_fee_commitment_tests" cargo test -- --test-threads=1 | ||
| eclair-interop-test: |
There was a problem hiding this comment.
Hmm, any chance we could make this an interop test for 0fc channels, not a 0fc test that does interop? I.e., can this be (a cfg-gated) part of the regular eclair tests, where we already have the docker setup etc.?
In 2024749, we started taking one slot in the package queue for each transaction broadcasted by the wallet upon `WalletEvent::ChainTipChanged`, so we increase the number of slots available in the queue.
Co-Authored-By: HAL 9000
This is particularly relevant for the electrum chain source; if we fail to fetch feerates, or zero fee commitments validation fails, and we do not stop the electrum chain source before returning an error, then the user will hit a debug assertion on the next restart.
Co-Authored-By: HAL 9000
Require Electrum and Esplora zero-fee commitments validation to observe the Bitcoin Core v29+ failure shape for the dummy TRUC package, instead of accepting any structured submitpackage response. Set the locktime field of the transaction to zero so that the test works at any chain-height, which is particularly helpful when starting ldk-node against test networks. Map HTTP 400 errors returned to `ChainSourceNotSupported` as this error code is returned by blockstream-electrs and mempool-electrs when running against Bitcoin Core v26. We previously would map this error to a general `ConnectionFailed` error, which is not correct for Bitcoin Core v26. Co-Authored-By: HAL 9000
Add a UniFFI wrapper so bindings can inspect channel type flags. Update anchor accounting tests to use channel type features instead of inferring zero-fee commitments from the commitment feerate. Co-Authored-By: HAL 9000
Co-Authored-By: HAL 9000
d30080b to
edf8512
Compare
edf8512 to
bda78cc
Compare
3 participants
No description provided.