Make mining more flexible in bdk_testenv#2100
Make mining more flexible in bdk_testenv#2100evanlinjin wants to merge 1 commit intobitcoindevkit:masterfrom
bdk_testenv#2100Conversation
evanlinjin
force-pushed
the
feature/testenv-mine-with-timestamp
branch
2 times, most recently
from
46db014 to
b91363e
Compare
|
@evanlinjin I'd suggest you start working on this on top of #1826, as we're migrating to |
evanlinjin
force-pushed
the
feature/testenv-mine-with-timestamp
branch
from
b91363e to
70bca59
Compare
evanlinjin
force-pushed
the
feature/testenv-mine-with-timestamp
branch
from
70bca59 to
8a31d1f
Compare
Good point. I'll do so once that PR is merged. I would like to test this against |
evanlinjin
force-pushed
the
feature/testenv-mine-with-timestamp
branch
5 times, most recently
from
f776d50 to
9d7f859
Compare
oleonardolima
left a comment
oleonardolima
left a comment
There was a problem hiding this comment.
utACK 9d7f859
Overall looks good. I left a few comments that might help with removing some decoding done here, didn't tried the changes yet though.
evanlinjin
force-pushed
the
feature/testenv-mine-with-timestamp
branch
from
9d7f859 to
2d3b817
Compare
crates/testenv/src/lib.rs
Outdated
| bdk_chain::bitcoin::script::Builder::new() | ||
| .push_int(bt.height as i64) | ||
| .push_opcode(bdk_chain::bitcoin::opcodes::OP_0) | ||
| .into_script() |
There was a problem hiding this comment.
question: shouldn't this one also have the coinbase_aux values ?
There was a problem hiding this comment.
I think this whole coinbase_scriptsig codeblock could be reworked, maybe even be extracted to a helper fn.
There was a problem hiding this comment.
Woops, I over-relied on Claude here.
There was a problem hiding this comment.
@oleonardolima Fixed it. Let me know what you think.
…dress Refactor block mining in `TestEnv` to use `getblocktemplate` RPC properly: - Add `MineParams` struct to configure mining (empty blocks, custom timestamp, custom coinbase address) - Add `mine_block()` method that builds blocks from the template with proper BIP34 coinbase scriptSig, witness commitment, and merkle root - Add `min_time_for_next_block()` and `get_block_template()` helpers - Refactor `mine_empty_block()` to use the new `mine_block()` API - Include mempool transactions when `empty: false`
evanlinjin
force-pushed
the
feature/testenv-mine-with-timestamp
branch
from
2d3b817 to
9620a7d
Compare
| /// Get the minimum valid timestamp for the next block. | ||
| pub fn min_time_for_next_block(&self) -> anyhow::Result<u32> { | ||
| Ok(self.get_block_template()?.min_time) | ||
| } |
There was a problem hiding this comment.
nit: currently it's only being used on the tests, is it really needed ? I mean, the same can be achieved directly calling the getblocktemplate.
| let coinbase_scriptsig = { | ||
| let mut script = build_coinbase_scriptsig(&bt, false); | ||
| // Ensure scriptSig is at least 2 bytes (pad with OP_0 if needed) | ||
| if script.len() < 2 { | ||
| script = build_coinbase_scriptsig(&bt, true); | ||
| }; | ||
| script |
There was a problem hiding this comment.
nit: it's only being used here, so the padding could be handled directly, but it's not a blocker though.
| let coinbase_outputs = if params.empty { | ||
| let tx_fees: Amount = bt | ||
| .transactions | ||
| .iter() | ||
| .map(|tx| tx.fee.to_unsigned().expect("fee must be positive")) | ||
| .sum(); | ||
| let value = bt | ||
| .coinbase_value | ||
| .to_unsigned() | ||
| .expect("coinbase_value must be positive") | ||
| - tx_fees; | ||
| vec![TxOut { | ||
| value, | ||
| script_pubkey: params.address_or_anyone_can_spend(), | ||
| }] | ||
| } else { | ||
| core::iter::once(TxOut { | ||
| value: bt | ||
| .coinbase_value | ||
| .to_unsigned() | ||
| .expect("coinbase_value must be positive"), | ||
| script_pubkey: params.address_or_anyone_can_spend(), | ||
| }) | ||
| .chain( | ||
| bt.default_witness_commitment | ||
| .as_ref() | ||
| .map(|s| -> Result<_, HexToBytesError> { | ||
| Ok(TxOut { | ||
| value: Amount::ZERO, | ||
| script_pubkey: ScriptBuf::from_hex(s)?, | ||
| }) | ||
| }) | ||
| .transpose()?, | ||
| ) | ||
| .collect() |
There was a problem hiding this comment.
question: was this created by claude, or there's an specific reason you need this ?
2 participants
Description
Add
mine_blocktobdk_testenv::Envwith custom timestamp and coinbase address. This allows us to test timelocked transactions.Changelog notice
Checklists
All Submissions:
New Features: