lntest: add bitcoind miner backend#10481
Conversation
Summary of ChangesHello @bhandras, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a bitcoind-backed miner implementation for lntest, which is a significant and well-executed enhancement. It allows for more realistic integration testing of Bitcoin Core-specific behaviors. The new miner abstraction is clean, and the refactoring of existing code to use it is thorough. My review includes one suggestion to improve maintainability by reducing code duplication in one of the modified files.
bhandras
force-pushed
the
lntest-bitcoind-miner
branch
from
524274d to
0872272
Compare
bhandras
force-pushed
the
lntest-bitcoind-miner
branch
from
2a17ef9 to
a8a24ed
Compare
bhandras
force-pushed
the
lntest-bitcoind-miner
branch
from
a8a24ed to
d2b5d3c
Compare
| // miner while there are already blocks present, which will take a bit | ||
| // longer than the 1 second the default settings amount to. Doubling |
There was a problem hiding this comment.
Nit: the sentence “which will take a bit longer than the 1 second the default settings amount to” is a bit unclear.
Maybe rephrase to something like “which can take longer than the default 1-second timeout”.
lntest/miner/bitcoind_miner.go
Outdated
| // 100 + numMatureOutputs | ||
| // blocks. |
There was a problem hiding this comment.
Nit: minor wording/style suggestion — consider writing “we mine 100 + numMatureOutputs blocks” inline.
|
@Roasbeef: review reminder |
saubyk
requested review from
ellemouton and
ziggie1984
and removed request for
Roasbeef and
yyforyongyu
|
|
||
| if err != nil { |
There was a problem hiding this comment.
dont think it is possible to get here
There was a problem hiding this comment.
Yeah good catch, retry loop is now cleaned up a bit.
lntest/miner/bitcoind_miner.go
Outdated
| return fmt.Errorf("unable to create wallet: %w", err) | ||
| } | ||
|
|
||
| if setupChain { |
There was a problem hiding this comment.
nit:
if !setupChain {
return
}
bloop blaap
make/testing_flags.mk
Outdated
| ITEST_FLAGS += -dbbackend=$(dbbackend) | ||
| endif | ||
|
|
||
| # Select miner backend independently from chain backend. |
There was a problem hiding this comment.
maybe add a comment that the default is always btcd ?
There was a problem hiding this comment.
Added that note here too: the make comment now says the miner backend selection is independent from the chain backend and still defaults to btcd.
lntest/miner/bitcoind_miner.go
Outdated
| feeRate btcutil.Amount) (*wire.MsgTx, error) { | ||
|
|
||
| // Extract destinations (address -> amount) in BTC. | ||
| destinations := make(map[string]json.RawMessage, len(outputs)) |
There was a problem hiding this comment.
think there is risk of override here if an output has same address
There was a problem hiding this comment.
Ah yes this was pretty bad, some early claude creation. Fixed it up a bit.
lntest/miner/miner.go
Outdated
|
|
||
| // Harness is the btcd-specific harness. This is only set when using | ||
| // the btcd backend, and is nil when using bitcoind. | ||
| *rpctest.Harness |
There was a problem hiding this comment.
should we make this named & unexported now? else it exposes all btcd backend calls even when nil
bhandras
force-pushed
the
lntest-bitcoind-miner
branch
3 times, most recently
from
7974fba to
f242f96
Compare
bhandras
force-pushed
the
lntest-bitcoind-miner
branch
from
f242f96 to
052ec49
Compare
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Thank you for adding this functionality.
Had some suggestions and minor comments.
| ) | ||
|
|
||
| return err | ||
| } |
There was a problem hiding this comment.
ConnectMiner: addnode onetry doesn't wait for peer establishment
addnode onetry returns immediately — the TCP handshake may not complete for another few hundred milliseconds. By contrast, BitcoindBackendConfig.ConnectMiner in bitcoind_common.go (added in this same PR) correctly polls getpeerinfo until the peer appears.
In SpawnTempMiner, if both miners are at the same genesis height (temp miner started with setupChain=false), the subsequent AssertMinerBlockHeightDelta(0) trivially passes without any actual P2P connection having succeeded. The DisconnectMiner that follows may then fail or silently no-op on a peer that was never connected, leaving the miners accidentally in sync during the reorg test.
Could you mirror the wait.NoError(getpeerinfo, ...) polling pattern from BitcoindBackendConfig.ConnectMiner here?
There was a problem hiding this comment.
Fixed. The bitcoind miner path now waits for the peer to show up in getpeerinfo after addnode onetry, matching the polling we already use for the bitcoind chain backend. That closes the gap where the height check could pass before the p2p connection was actually established.
| height, err := b.rpcClient.GetBlockCount() | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } |
There was a problem hiding this comment.
GetBestBlock: two RPC calls are not atomic
GetBestBlockHash and GetBlockCount are separate round-trips. If a block is mined between the two calls the function returns the hash of block N but the height of block N+1. The btcd backend gets both in a single atomic call.
getblockchaininfo returns bestblockhash and blocks in one response and would fix this. In practice this race is unlikely in tests, but it's easy to fix.
There was a problem hiding this comment.
Fixed. GetBestBlock now uses getblockchaininfo so the hash and height come back from a single RPC response instead of two separate round-trips.
lntest/miner/miner.go
Outdated
| err = rpctest.JoinNodes(nodeSlice, rpctest.Blocks) | ||
| require.NoError(err, "unable to join node on blocks") | ||
| } else { | ||
| h.AssertMinerBlockHeightDelta(tempMiner, 0) |
There was a problem hiding this comment.
Redundant AssertMinerBlockHeightDelta call
When the bitcoind backend is active, h.harness and tempMiner.harness are both nil, so the else branch here runs AssertMinerBlockHeightDelta(tempMiner, 0), and then line 717 runs it again unconditionally. The inner call (this line) is redundant — the outer one on line 717 is sufficient.
There was a problem hiding this comment.
Fixed. I removed the inner AssertMinerBlockHeightDelta(tempMiner, 0) and keep only the unconditional check after the backend-specific sync path.
| // | ||
| // For the btcd backend, these parameters map to rpctest.Harness.SetUp. | ||
| // For other backends, these parameters can be ignored. | ||
| Start(setupChain bool, numMatureOutputs uint32) error |
There was a problem hiding this comment.
The comment says the parameters can be ignored by non-btcd backends, but the bitcoind implementation actually uses both — setupChain (line 230 of bitcoind_miner.go) and numMatureOutputs (line 272, mines 100 + numMatureOutputs blocks). The comment was likely written before the bitcoind backend was fully implemented and never updated. Suggest replacing with a description of what the parameters mean rather than how each backend handles them.
There was a problem hiding this comment.
Fixed. I updated the Start comment to describe what setupChain and numMatureOutputs mean instead of implying non-btcd backends can ignore them.
| // for compatibility with existing patterns in lntest. | ||
| //nolint:containedctx | ||
| runCtx context.Context | ||
| cancel context.CancelFunc |
There was a problem hiding this comment.
nit: Both backends create a child context with context.WithCancel but never actually read runCtx back — only cancel() is called in Stop(). The context is stored purely so HarnessMiner can copy it and pass it to temp miner constructors in SpawnTempMiner.
A simpler design would be to not store runCtx in the backends at all, have HarnessMiner hold the parent context directly, and let each backend derive its own cancel internally. Not blocking, just worth considering in a follow-up cleanup.
There was a problem hiding this comment.
Agree this can be simplified further. I left it out of this PR to keep the review fixes scoped to behavior and documentation changes, but it would make sense as follow-up cleanup.
| // NewMinerWithConfig creates a new miner with the specified configuration. | ||
| // The Backend field in the config determines which backend to use ("btcd" or | ||
| // "bitcoind"). | ||
| func NewMinerWithConfig(ctxt context.Context, t *testing.T, |
There was a problem hiding this comment.
nit: The WithConfig suffix feels redundant on a constructor — if config is needed, you just pass it. NewMinerWithConfig could simply be NewMiner, with the current no-arg NewMiner becoming NewBtcdMiner for symmetry with NewBitcoindMiner.
Related: newBitcoindMiner is a trivial wrapper that just builds a MinerConfig and immediately delegates to newBitcoindMinerWithConfig. The private WithConfig layer could be collapsed — one private constructor per backend that takes a config, no intermediate wrapper needed. Only two call sites outside the package so the rename is a small change.
There was a problem hiding this comment.
I agree there is room to simplify the constructor naming. I left that out of this PR so I wouldn't mix a broader API rename into the bitcoind miner change set, but I'm happy to do it as follow-up cleanup.
|
|
||
| // GetRawTransactionVerbose makes a RPC call to the miner's | ||
| // GetRawTransactionVerbose and asserts. | ||
| func (h *HarnessMiner) GetRawTransactionVerbose( |
There was a problem hiding this comment.
does the getrawtransaction bitcoind call not support like some function arguments to return the transaction in verbose mode ? I remember bitcoin-cli getrawtransaction 1 txid
There was a problem hiding this comment.
Yep, good point. The shared rpcclient supports the verbose getrawtransaction call for bitcoind as well, so I wired GetRawTransactionVerbose through the miner backend instead of special-casing btcd.
make/testing_flags.mk
Outdated
| ITEST_FLAGS += -dbbackend=$(dbbackend) | ||
| endif | ||
|
|
||
| # Select miner backend independently from chain backend. |
There was a problem hiding this comment.
maybe add a comment that the default is always btcd ?
itest/lnd_test.go
Outdated
| ) | ||
|
|
||
| // minerBackendFlag selects which miner backend to use. If not set, a | ||
| // default miner is used. |
There was a problem hiding this comment.
maybe add a comment which the default miner is, its btcd ?
There was a problem hiding this comment.
Added that clarification in the flag comment as well. If -minerbackend is not set, the default miner remains btcd.
| - name: bitcoind | ||
| args: backend=bitcoind cover=1 | ||
| - name: bitcoind-miner | ||
| args: backend=bitcoind minerbackend=bitcoind cover=1 |
There was a problem hiding this comment.
nit (non-blocking): The bitcoind-miner CI job still builds and passes -btcdexec even though minerbackend=bitcoind never uses it. The btcd binary is built and linked unconditionally for all itest runs regardless of miner backend. Could be worth skipping the btcd build step when minerbackend=bitcoind to save a bit of CI time, but fine to address in a follow-up.
There was a problem hiding this comment.
Agreed. I left the btcd build optimization out of this PR because it changes the itest job shape rather than the miner behavior itself, but it seems like a reasonable follow-up CI cleanup.
bhandras
force-pushed
the
lntest-bitcoind-miner
branch
from
052ec49 to
53831f0
Compare
Introduce a miner backend interface and implement both btcd and bitcoind-backed miners for lntest. This lets the harness drive mining and mempool assertions using bitcoind in addition to btcd. Also update harness helpers to avoid btcd-only assumptions (network params, raw tx submission, funding shim output index lookup) and make bitcoind miner disconnects more reliable.
Add an itest flag to choose the miner backend (btcd vs bitcoind) and provide a build-tag default so that `-tags=bitcoind` naturally uses a bitcoind miner. Wire the flag through `make testing_flags.mk` so callers can set `minerbackend=bitcoind` independently of the chain backend.
Add a basic itest matrix entry that sets minerbackend=bitcoind alongside backend=bitcoind, ensuring CI covers the bitcoind miner path.
bhandras
force-pushed
the
lntest-bitcoind-miner
branch
from
53831f0 to
ad62f5f
Compare
6 participants
Summary
This PR extends
lntestwith abitcoind-backed miner implementation. Until now,lntestassumed abtcdminer (rpctest harness) even when the chain backend wasbitcoind, which made it hard to exercise Bitcoin Core-specificmining and mempool behavior.
A key motivation for this work is enabling
lntestto support mining and validating v3 packages (and other Core policy/package-relay behaviors) in integration tests. Those workflows are most realistically exercised with Bitcoin Core as the miner.What’s included
lntest/minerwith two concrete backends:btcdbackend (wrapping existingrpctest.Harnessusage)bitcoindbackend (spawns a localbitcoindon regtest and implements the miner APIs needed bylntest)-minerbackenditest flag so the miner backend can be selected independently from the chain backend.backend=bitcoindandminerbackend=bitcoindto ensure we exercise the new path.Why this matters for v3 packages
Package relay / v3 transaction workflows are fundamentally Bitcoin Core policy and mining driven. Having
lntestable to:is a prerequisite for writing reliable integration coverage around v3 package construction, acceptance, and mining behavior.
How to run
Locally:
make itest backend=bitcoind minerbackend=bitcoindCI will run an additional job (matrix entry) with the same arguments.
Notes
make lintpasses.