💬 frankomosh commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2533161038)
> yes but its better to roll up responsibility to the object that holds it? Can you point out exactly where maybe? so we can see? if not, this seems like a better approach to me.
I think rolling the witness into `CTxIn::ToString()` is fine only if you keep the old separate witness block in `CTransaction::ToString()` (or try adding a flag to suppress it). Right now you’ve deleted that block, so every existing log, test and user script would lose the witness section.
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2533161038)
> yes but its better to roll up responsibility to the object that holds it? Can you point out exactly where maybe? so we can see? if not, this seems like a better approach to me.
I think rolling the witness into `CTxIn::ToString()` is fine only if you keep the old separate witness block in `CTransaction::ToString()` (or try adding a flag to suppress it). Right now you’ve deleted that block, so every existing log, test and user script would lose the witness section.
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3540669363)
@ajtowns the `CoinbaseTemplate` struct introduced by this PR closely matches the [NewTemplate message](https://stratumprotocol.org/specification/07-Template-Distribution-Protocol/#72-newtemplate-server-client) in Stratum v2. The main difference is that the `coinbase_tx_outputs` field of `NewTemplate` uses an ad hoc serialisation that I don't think we should embrace, so I keep the outputs in a vector.
Note that the code in `ExtractCoinbaseTemplate()` was mostly copied from `sv2-tp`. So the mai
...
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3540669363)
@ajtowns the `CoinbaseTemplate` struct introduced by this PR closely matches the [NewTemplate message](https://stratumprotocol.org/specification/07-Template-Distribution-Protocol/#72-newtemplate-server-client) in Stratum v2. The main difference is that the `coinbase_tx_outputs` field of `NewTemplate` uses an ad hoc serialisation that I don't think we should embrace, so I keep the outputs in a vector.
Note that the code in `ExtractCoinbaseTemplate()` was mostly copied from `sv2-tp`. So the mai
...
💬 Sjors commented on pull request "test: Fix race condition in IPC interface block progation test":
(https://github.com/bitcoin/bitcoin/pull/33880#discussion_r2533275932)
Maybe just move this line below sync_all.
(https://github.com/bitcoin/bitcoin/pull/33880#discussion_r2533275932)
Maybe just move this line below sync_all.
💬 Sjors commented on pull request "test: Fix race condition in IPC interface block progation test":
(https://github.com/bitcoin/bitcoin/pull/33880#issuecomment-3540715551)
Good catch. The test needs to check two things:
1. The IPC node actually updates it own chain after `submitBlock()`
2. The other node accepts the block
So the check for `current_block_height + 1` is important, but we should either do it for `self.nodes[0]` (no need to `wait_until`) _or_ keep it for `self.nodes[1]`, but after `sync_all`.
Only relying on `sync_all` is not enough, because it won't catch a regression where the IPC code fails to update the node itself, so there's no block t
...
(https://github.com/bitcoin/bitcoin/pull/33880#issuecomment-3540715551)
Good catch. The test needs to check two things:
1. The IPC node actually updates it own chain after `submitBlock()`
2. The other node accepts the block
So the check for `current_block_height + 1` is important, but we should either do it for `self.nodes[0]` (no need to `wait_until`) _or_ keep it for `self.nodes[1]`, but after `sync_all`.
Only relying on `sync_all` is not enough, because it won't catch a regression where the IPC code fails to update the node itself, so there's no block t
...
💬 cedwies commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#issuecomment-3540768449)
utACK d442598
(https://github.com/bitcoin/bitcoin/pull/33680#issuecomment-3540768449)
utACK d442598
👍 TheCharlatan approved a pull request: "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`"
(https://github.com/bitcoin/bitcoin/pull/31823#pullrequestreview-3471932363)
ACK a7c96f874de13ace9814da92fd6160a126a97ebf
(https://github.com/bitcoin/bitcoin/pull/31823#pullrequestreview-3471932363)
ACK a7c96f874de13ace9814da92fd6160a126a97ebf
💬 fanquake commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3540923567)
Also tested on aarch64 with Debian 13.x.
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3540923567)
Also tested on aarch64 with Debian 13.x.
👍 TheCharlatan approved a pull request: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3471964192)
ACK 96963b888e5a10f4024fa0449c60c02e3bed6245
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3471964192)
ACK 96963b888e5a10f4024fa0449c60c02e3bed6245
💬 fanquake commented on pull request "doc: Update NetBSD Build Guide":
(https://github.com/bitcoin/bitcoin/pull/33876#issuecomment-3540993627)
ACK c29eaeeaf9377b1e3c33e82d29597790a5388403
(https://github.com/bitcoin/bitcoin/pull/33876#issuecomment-3540993627)
ACK c29eaeeaf9377b1e3c33e82d29597790a5388403
🚀 fanquake merged a pull request: "doc: Update NetBSD Build Guide"
(https://github.com/bitcoin/bitcoin/pull/33876)
(https://github.com/bitcoin/bitcoin/pull/33876)
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3541021155)
@plebhash wrote:
> however, we always need to call getBlock specifically because it contains the header + txdata of the template... and as a byproduct, we happen to also get the Coinbase Tx data, which saves us one extra call to getCoinbase/getCoinbaseTx
Regarding the header, there's a separate `getHeader()` method.
It's true that you need txdata anyway, but you don't need it immediately. You need it when the pool asks using the `RequestTransactionData` message.
When the node and the
...
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3541021155)
@plebhash wrote:
> however, we always need to call getBlock specifically because it contains the header + txdata of the template... and as a byproduct, we happen to also get the Coinbase Tx data, which saves us one extra call to getCoinbase/getCoinbaseTx
Regarding the header, there's a separate `getHeader()` method.
It's true that you need txdata anyway, but you don't need it immediately. You need it when the pool asks using the `RequestTransactionData` message.
When the node and the
...
🚀 fanquake merged a pull request: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537)
(https://github.com/bitcoin/bitcoin/pull/33537)
🤔 janb84 reviewed a pull request: "kernel: Expose `CheckTransaction` consensus validation function"
(https://github.com/bitcoin/bitcoin/pull/33796#pullrequestreview-3472013748)
Concept ACK a262282abefc0a16be329ab2179c193b4b32cc5b
PR extends bitcoin kernel library with the checkTransaction consensus validation function.
Added a suggestion NIT (non-blocking) and I agree with the NIT from [yuvicc](https://github.com/bitcoin/bitcoin/pull/33796/files#r2506321358)
Steps taken:
- Build & tested
- Code review
- Create a implementation PR for [BitcoinKernel.Net](https://github.com/janb84/BitcoinKernel.NET/pull/2)
(https://github.com/bitcoin/bitcoin/pull/33796#pullrequestreview-3472013748)
Concept ACK a262282abefc0a16be329ab2179c193b4b32cc5b
PR extends bitcoin kernel library with the checkTransaction consensus validation function.
Added a suggestion NIT (non-blocking) and I agree with the NIT from [yuvicc](https://github.com/bitcoin/bitcoin/pull/33796/files#r2506321358)
Steps taken:
- Build & tested
- Code review
- Create a implementation PR for [BitcoinKernel.Net](https://github.com/janb84/BitcoinKernel.NET/pull/2)
💬 janb84 commented on pull request "kernel: Expose `CheckTransaction` consensus validation function":
(https://github.com/bitcoin/bitcoin/pull/33796#discussion_r2533510544)
NIT: (non-blocking) maybe add a note why only the 2 (possible) states are tested ? For a non-bitcoin-core dev this is maybe a bit unclear given all the TxValidationResults and the Check_Transaction that these are the 2 outcomes.
It's for me not logical to add this as a comment on the function because it returns a pointer to the results.
```suggestion
// -----------------------------------------------------------------------------
// Note: CheckTransaction performs only basic consensus ch
...
(https://github.com/bitcoin/bitcoin/pull/33796#discussion_r2533510544)
NIT: (non-blocking) maybe add a note why only the 2 (possible) states are tested ? For a non-bitcoin-core dev this is maybe a bit unclear given all the TxValidationResults and the Check_Transaction that these are the 2 outcomes.
It's for me not logical to add this as a comment on the function because it returns a pointer to the results.
```suggestion
// -----------------------------------------------------------------------------
// Note: CheckTransaction performs only basic consensus ch
...
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533559315)
I think it's good to have `parse_and_deserialize_coinbase` be safe to use, because we might eventually move it into the test framework as IPC coverage increases. I'll add some documentation.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533559315)
I think it's good to have `parse_and_deserialize_coinbase` be safe to use, because we might eventually move it into the test framework as IPC coverage increases. I'll add some documentation.
💬 fanquake commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2533590975)
```bash
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin-cli
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin-qt
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin-tx
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin-ut
...
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2533590975)
```bash
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin-cli
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin-qt
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin-tx
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin-ut
...
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533615466)
I don't think miners use the full `scriptSig` so if we slightly go over it, it shouldn't immediately result in invalid blocks. So it seems overkill to abort the template generation entirely.
I'll log a warning.
The main purpose of this `Assume` is to remind ourselves to inform the mining ecosystem if we ever expand the scriptSig prefix substantially.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533615466)
I don't think miners use the full `scriptSig` so if we slightly go over it, it shouldn't immediately result in invalid blocks. So it seems overkill to abort the template generation entirely.
I'll log a warning.
The main purpose of this `Assume` is to remind ourselves to inform the mining ecosystem if we ever expand the scriptSig prefix substantially.
🚀 fanquake merged a pull request: "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings"
(https://github.com/bitcoin/bitcoin/pull/33247)
(https://github.com/bitcoin/bitcoin/pull/33247)
👋 fanquake's pull request is ready for review: "depends: update xcb-util packages to latest versions"
(https://github.com/bitcoin/bitcoin/pull/33851)
(https://github.com/bitcoin/bitcoin/pull/33851)
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533642433)
I also clarified the comment that it might be a _silently_ breaking change. Although the spec requires `NewTemplate` to keep it at max 8 bytes, the serialisation supports up to 256 and a Template Provider implementation might just not check / enforce this.
https://stratumprotocol.org/specification/07-Template-Distribution-Protocol/#72-newtemplate-server-client
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533642433)
I also clarified the comment that it might be a _silently_ breaking change. Although the spec requires `NewTemplate` to keep it at max 8 bytes, the serialisation supports up to 256 and a Template Provider implementation might just not check / enforce this.
https://stratumprotocol.org/specification/07-Template-Distribution-Protocol/#72-newtemplate-server-client