💬 ismaelsadeeq commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514611258)
If you want to modify the template, yes. But for things like sendtemplate, fee estimation, that just want to retrieve data from the template they don't need a copy.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514611258)
If you want to modify the template, yes. But for things like sendtemplate, fee estimation, that just want to retrieve data from the template they don't need a copy.
📝 fanquake converted_to_draft a pull request: "depends: update xcb-util packages to latest versions"
(https://github.com/bitcoin/bitcoin/pull/33851)
Based on #33537.
(https://github.com/bitcoin/bitcoin/pull/33851)
Based on #33537.
📝 stickies-v opened a pull request: "kernel: add btck_block_tree_entry_equals"
(https://github.com/bitcoin/bitcoin/pull/33855)
`BlockTreeEntry` objects are often compared. This happens frequently in our own codebase and seems likely to be the case for clients, too. Users can already work around this by comparing based on block hash (and optionally height as belt-and-suspenders), but I think this should be part of the interface for performance and consistency reasons.
Note: perhaps this is too ad-hoc, and we should extend this PR to add the operator for more types? `BlockTreeEntry` is the main one I've needed this for
...
(https://github.com/bitcoin/bitcoin/pull/33855)
`BlockTreeEntry` objects are often compared. This happens frequently in our own codebase and seems likely to be the case for clients, too. Users can already work around this by comparing based on block hash (and optionally height as belt-and-suspenders), but I think this should be part of the interface for performance and consistency reasons.
Note: perhaps this is too ad-hoc, and we should extend this PR to add the operator for more types? `BlockTreeEntry` is the main one I've needed this for
...
👋 waketraindev's pull request is ready for review: "Prevent re-execution of sensitive commands from console history"
(https://github.com/bitcoin-core/gui/pull/909)
(https://github.com/bitcoin-core/gui/pull/909)
💬 ryanofsky commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517459399)
re: https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517090033
Thanks @TheCharlatan. I think the approach you took in #30595 of making the kernel API mirror the current `logging.h` API was the best one to take as a starting point. #30595 covers a large surface area, and it would have been asking too much of reviewers to evaluate all the different ways the kernel API and existing APIs could or should diverge in one PR. It should be more manageable to make kernel API improvements in
...
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517459399)
re: https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517090033
Thanks @TheCharlatan. I think the approach you took in #30595 of making the kernel API mirror the current `logging.h` API was the best one to take as a starting point. #30595 covers a large surface area, and it would have been asking too much of reviewers to evaluate all the different ways the kernel API and existing APIs could or should diverge in one PR. It should be more manageable to make kernel API improvements in
...
💬 fanquake commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3517462890)
Note that this will still fail:
```bash
[ 74%] Building CXX object src/test/kernel/CMakeFiles/test_kernel.dir/test_kernel.cpp.o
/bitcoin/src/test/kernel/test_kernel.cpp: In member function 'void btck_transaction_tests::test_method()':
/bitcoin/src/test/kernel/test_kernel.cpp:448:34: error: no match for 'operator|' (operand types are 'btck::Range<btck::Transaction, &btck::TransactionApi<btck::Transaction>::CountOutputs, &btck::TransactionApi<btck::Transaction>::GetOutput>' and 'std::ranges::views
...
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3517462890)
Note that this will still fail:
```bash
[ 74%] Building CXX object src/test/kernel/CMakeFiles/test_kernel.dir/test_kernel.cpp.o
/bitcoin/src/test/kernel/test_kernel.cpp: In member function 'void btck_transaction_tests::test_method()':
/bitcoin/src/test/kernel/test_kernel.cpp:448:34: error: no match for 'operator|' (operand types are 'btck::Range<btck::Transaction, &btck::TransactionApi<btck::Transaction>::CountOutputs, &btck::TransactionApi<btck::Transaction>::GetOutput>' and 'std::ranges::views
...
📝 Eunovo converted_to_draft a pull request: "validation: fix assumevalid is ignored during reindex"
(https://github.com/bitcoin/bitcoin/pull/33854)
During a reindex, the assumevalid setting may be ignored if the chainwork of the best header is below `minimumchainwork`. This happens when the previous IBD was stopped before connecting enough blocks to reach the required `minimumchainwork`. The assumevalid block will also be missing from the block index because the assumevalid block is set to match the `minimumchainwork`. See [Issue #31494](https://github.com/bitcoin/bitcoin/issues/31494).
As a result, `reindex` can take significantly longe
...
(https://github.com/bitcoin/bitcoin/pull/33854)
During a reindex, the assumevalid setting may be ignored if the chainwork of the best header is below `minimumchainwork`. This happens when the previous IBD was stopped before connecting enough blocks to reach the required `minimumchainwork`. The assumevalid block will also be missing from the block index because the assumevalid block is set to match the `minimumchainwork`. See [Issue #31494](https://github.com/bitcoin/bitcoin/issues/31494).
As a result, `reindex` can take significantly longe
...
🤔 stickies-v reviewed a pull request: "kernel: Add block header support and validation"
(https://github.com/bitcoin/bitcoin/pull/33822#pullrequestreview-3448629035)
Approach ACK 86958ee668a950a03d08ef188f2de27137e89b33
I think the commit can be broken up a bit further, e.g. validationstate logic and chainman logic can be separate I think?
(https://github.com/bitcoin/bitcoin/pull/33822#pullrequestreview-3448629035)
Approach ACK 86958ee668a950a03d08ef188f2de27137e89b33
I think the commit can be broken up a bit further, e.g. validationstate logic and chainman logic can be separate I think?
💬 stickies-v commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2514666155)
> header is not owned and depends on the lifetime of the block
I don't think that's correct?
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2514666155)
> header is not owned and depends on the lifetime of the block
I don't think that's correct?
💬 stickies-v commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2514692129)
Would be nice if the test tests the entire block header interface.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2514692129)
Would be nice if the test tests the entire block header interface.
💬 stickies-v commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2514681291)
This is marked as resolved, but I don't think this is resolved yet? I agree to keep the `state` name.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2514681291)
This is marked as resolved, but I don't think this is resolved yet? I agree to keep the `state` name.
💬 roconnor-blockstream commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3517541201)
I tried cargo culting @ajtowns 's testing code to check that currently one can create (multisig) UTXOs with hybrid keys, but cannot spend them:
self.log.info("Restarting node with -permitbaremultisig=1")
self.restart_node(0, extra_args=["-permitbaremultisig=1"])
self.log.info('Creating hybrid txs is standard, but spending is non-standard')
address = self.wallet.get_address()
tx = self.wallet.create_self_transfer()['tx'] # Pick a random coin(base)
...
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3517541201)
I tried cargo culting @ajtowns 's testing code to check that currently one can create (multisig) UTXOs with hybrid keys, but cannot spend them:
self.log.info("Restarting node with -permitbaremultisig=1")
self.restart_node(0, extra_args=["-permitbaremultisig=1"])
self.log.info('Creating hybrid txs is standard, but spending is non-standard')
address = self.wallet.get_address()
tx = self.wallet.create_self_transfer()['tx'] # Pick a random coin(base)
...
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2514733608)
in a6c9f19c7d523fd6c2573e47e1992448b76f1977
Note to self/reviewers: we can't assert that the feerate diagram's last value matches cached `txTotalSize`, because it converts from weight to vsize happens per-chunk. These chunk vsizes can be smaller than the summed individual vsizes of the transactions.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2514733608)
in a6c9f19c7d523fd6c2573e47e1992448b76f1977
Note to self/reviewers: we can't assert that the feerate diagram's last value matches cached `txTotalSize`, because it converts from weight to vsize happens per-chunk. These chunk vsizes can be smaller than the summed individual vsizes of the transactions.
💬 jonatack commented on pull request "doc: document fingerprinting risk when operating node on multiple networks":
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2514752164)
> This combination, IMO, poses less risk to the privacy-concerned individual as they are exposing their node over two privacy networks.
Perhaps clarify that in the documentation, as those that do run over both Tor and I2P could conclude that they risk being fingerprinted in the same way as described here.
I could be wrong, but I don't see who would listen on both clearnet and Tor (or I2P) if they are concerned about privacy. ISTM those who do that are willingly providing a service to the n
...
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2514752164)
> This combination, IMO, poses less risk to the privacy-concerned individual as they are exposing their node over two privacy networks.
Perhaps clarify that in the documentation, as those that do run over both Tor and I2P could conclude that they risk being fingerprinted in the same way as described here.
I could be wrong, but I don't see who would listen on both clearnet and Tor (or I2P) if they are concerned about privacy. ISTM those who do that are willingly providing a service to the n
...
💬 maflcko commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3517608583)
The unused format include is removed in https://github.com/bitcoin/bitcoin/pull/33853/files and the rvalue ranges thing (ref https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113154) is "fixed" in https://github.com/bitcoin/bitcoin/pull/33842.
An alternative could be to use an lvalue. However, I am not sure if this is worth it:
```diff
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index d9875ee..6ef494b 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/ker
...
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3517608583)
The unused format include is removed in https://github.com/bitcoin/bitcoin/pull/33853/files and the rvalue ranges thing (ref https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113154) is "fixed" in https://github.com/bitcoin/bitcoin/pull/33842.
An alternative could be to use an lvalue. However, I am not sure if this is worth it:
```diff
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index d9875ee..6ef494b 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/ker
...
💬 maflcko commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2514804401)
would be nice to use clang-tidy named args for new code, instead of unnamed literal values
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2514804401)
would be nice to use clang-tidy named args for new code, instead of unnamed literal values
💬 fanquake commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3517687922)
https://github.com/bitcoin/bitcoin/actions/runs/19269384275/job/55093373232?pr=33819#step:7:3549:
```bash
Error: node0 2025-11-11T15:38:48.659039Z [unknown] [validation.cpp:4535] [bool ChainstateManager::ProcessNewBlock(const std::shared_ptr<const CBlock> &, bool, bool, bool *)] [error] ProcessNewBlock: AcceptBlock FAILED (bad-version(0x00000000), rejected nVersion=0x00000000 block)
```
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3517687922)
https://github.com/bitcoin/bitcoin/actions/runs/19269384275/job/55093373232?pr=33819#step:7:3549:
```bash
Error: node0 2025-11-11T15:38:48.659039Z [unknown] [validation.cpp:4535] [bool ChainstateManager::ProcessNewBlock(const std::shared_ptr<const CBlock> &, bool, bool, bool *)] [error] ProcessNewBlock: AcceptBlock FAILED (bad-version(0x00000000), rejected nVersion=0x00000000 block)
```
💬 tobtoht commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3517715698)
>Still some issues making libcxb fully static.
Just sharing this here in case it is useful: https://github.com/tobtoht/bitcoin/commit/24cfce443aedb2c7cb7ac3266029ad01af2ec47c
There probably is a better way to build `libxcb_util_image` or skip building the tests. Rather than editing these autogenerated scripts (and without reintroducing autotools).
```
$ lddtree bitcoin-qt
bitcoin-qt (interpreter => /lib64/ld-linux-x86-64.so.2)
libfontconfig.so.1 => /usr/lib/libfontconfig.so.1
...
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3517715698)
>Still some issues making libcxb fully static.
Just sharing this here in case it is useful: https://github.com/tobtoht/bitcoin/commit/24cfce443aedb2c7cb7ac3266029ad01af2ec47c
There probably is a better way to build `libxcb_util_image` or skip building the tests. Rather than editing these autogenerated scripts (and without reintroducing autotools).
```
$ lddtree bitcoin-qt
bitcoin-qt (interpreter => /lib64/ld-linux-x86-64.so.2)
libfontconfig.so.1 => /usr/lib/libfontconfig.so.1
...
🤔 l0rinc requested changes to a pull request: "fuzz: compact block harness"
(https://github.com/bitcoin/bitcoin/pull/33300#pullrequestreview-3447750850)
Concept and approach ACK, thanks for tackling this.
I like how you've extracted the first few commits that I could quickly get out of the way to continue to the more difficult ones - which are a bit too difficult though, it would help me personally if we could split it into smaller functional chunks, so that the reviewers are guided through the change. The commit messages could give extra context where needed. I don't think we should split by functions, but rather functionalities so that each c
...
(https://github.com/bitcoin/bitcoin/pull/33300#pullrequestreview-3447750850)
Concept and approach ACK, thanks for tackling this.
I like how you've extracted the first few commits that I could quickly get out of the way to continue to the more difficult ones - which are a bit too difficult though, it would help me personally if we could split it into smaller functional chunks, so that the reviewers are guided through the change. The commit messages could give extra context where needed. I don't think we should split by functions, but rather functionalities so that each c
...
💬 l0rinc commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2514060602)
I have the same problem in `71719d172e3a435dd983293fc9da6a08974f8b01` - we're introducing dead code, I don't have a way to judge if this is indeed the correct solution, since up to this point there wasn't any pain that this alleviates.
Looking at the commit I see that `setup_validation_interface_no_scheduler` is always false therefore we can delete the code - and I have to check the next commits and come back here to reevaluate that.
Can we add a simpler fuzz test which needs this in the same
...
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2514060602)
I have the same problem in `71719d172e3a435dd983293fc9da6a08974f8b01` - we're introducing dead code, I don't have a way to judge if this is indeed the correct solution, since up to this point there wasn't any pain that this alleviates.
Looking at the commit I see that `setup_validation_interface_no_scheduler` is always false therefore we can delete the code - and I have to check the next commits and come back here to reevaluate that.
Can we add a simpler fuzz test which needs this in the same
...