💬 maflcko commented on pull request "kernel: Allow null arguments for serialized data":
(https://github.com/bitcoin/bitcoin/pull/33853#discussion_r2514464091)
```suggestion
BOOST_CHECK_THROW(Transaction{std::span{static_cast<std::byte*>(nullptr), 2}}, std::runtime_error);
```
style-nit in 5b89956eeb76cf8c9717152fbb0928e026fc0087 (no need to re-touch), also I haven't tried compiling, but I think the compiler can derive the type here and it can be omitted. Same below.
(https://github.com/bitcoin/bitcoin/pull/33853#discussion_r2514464091)
```suggestion
BOOST_CHECK_THROW(Transaction{std::span{static_cast<std::byte*>(nullptr), 2}}, std::runtime_error);
```
style-nit in 5b89956eeb76cf8c9717152fbb0928e026fc0087 (no need to re-touch), also I haven't tried compiling, but I think the compiler can derive the type here and it can be omitted. Same below.
👍 maflcko approved a pull request: "kernel: Allow null arguments for serialized data"
(https://github.com/bitcoin/bitcoin/pull/33853#pullrequestreview-3448360329)
Make sense to handle empty spans at runtime properly via our serialization and the existing serialize-error-handling, to avoid runtime violations of the non-null attribute.
review ACK a3ac59a4316305fb38a5338b48940682889d0dc2 🥈
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_fou
...
(https://github.com/bitcoin/bitcoin/pull/33853#pullrequestreview-3448360329)
Make sense to handle empty spans at runtime properly via our serialization and the existing serialize-error-handling, to avoid runtime violations of the non-null attribute.
review ACK a3ac59a4316305fb38a5338b48940682889d0dc2 🥈
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_fou
...
🤔 mzumsande reviewed a pull request: "doc: document fingerprinting risk when operating node on multiple networks"
(https://github.com/bitcoin/bitcoin/pull/33750#pullrequestreview-3448401470)
ACK https://github.com/bitcoin/bitcoin/commit/e346ecae830e10310979e5f64de63e043a383ff1
nit: Commit separation is still not clean (second commit still touches tor.md).
(https://github.com/bitcoin/bitcoin/pull/33750#pullrequestreview-3448401470)
ACK https://github.com/bitcoin/bitcoin/commit/e346ecae830e10310979e5f64de63e043a383ff1
nit: Commit separation is still not clean (second commit still touches tor.md).
✅ fanquake closed an issue: "Fuzz: compare our AES implementation to AES-NI "
(https://github.com/bitcoin/bitcoin/issues/27548)
(https://github.com/bitcoin/bitcoin/issues/27548)
💬 fanquake commented on issue "Fuzz: compare our AES implementation to AES-NI ":
(https://github.com/bitcoin/bitcoin/issues/27548#issuecomment-3517243078)
Will close for now, and see what comes out of https://github.com/bitcoinfuzz/bitcoinfuzz/issues/321.
(https://github.com/bitcoin/bitcoin/issues/27548#issuecomment-3517243078)
Will close for now, and see what comes out of https://github.com/bitcoinfuzz/bitcoinfuzz/issues/321.
💬 mzumsande commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2514514693)
Sorry, I missed this. yes, it's a bit ambiguous, the intention is "required for `FindNestBlocks`" (it's also used as a starting point there). This could be made more precise in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2514514693)
Sorry, I missed this. yes, it's a bit ambiguous, the intention is "required for `FindNestBlocks`" (it's also used as a starting point there). This could be made more precise in a follow-up.
👋 fanquake's pull request is ready for review: "[29.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33611)
(https://github.com/bitcoin/bitcoin/pull/33611)
💬 laanwj commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517278954)
As long as these are linked dynamically, this will increase the minimum versions that need to be installed on the target system. i'm not sure how much of a problem this is, if we have a reason to need newer functions of these interfaces.
(at the least this will have to be tested on common target distro versions)
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517278954)
As long as these are linked dynamically, this will increase the minimum versions that need to be installed on the target system. i'm not sure how much of a problem this is, if we have a reason to need newer functions of these interfaces.
(at the least this will have to be tested on common target distro versions)
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3517285109)
CI failed because it runs against master and #33575 has been merged, which added `interruptWait` at position `@11`. I rebased and bumped `getCoinbase()` to `@12`.
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3517285109)
CI failed because it runs against master and #33575 has been merged, which added `interruptWait` at position `@11`. I rebased and bumped `getCoinbase()` to `@12`.
💬 fanquake commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517290596)
@laanwj my intention is to have these linked statically (within the v31 cycle), this has just been pulled out of #33537. If you'd prefer to review any changes as a single unit in #33537, I'm happy to keep everything together there.
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517290596)
@laanwj my intention is to have these linked statically (within the v31 cycle), this has just been pulled out of #33537. If you'd prefer to review any changes as a single unit in #33537, I'm happy to keep everything together there.
💬 laanwj commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517330032)
Right! It's a good idea to pull it out. My point is just that we probably only want to merge this after #33537.
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517330032)
Right! It's a good idea to pull it out. My point is just that we probably only want to merge this after #33537.
💬 Sjors commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514589535)
In other words, each newly requested temple is _copied_ from the cache? Sounds good.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514589535)
In other words, each newly requested temple is _copied_ from the cache? Sounds good.
💬 fanquake commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517375440)
> My point is just that we probably only want to merge this after https://github.com/bitcoin/bitcoin/pull/33537.
I'll invert the PR stack.
(https://github.com/bitcoin/bitcoin/pull/33851#issuecomment-3517375440)
> My point is just that we probably only want to merge this after https://github.com/bitcoin/bitcoin/pull/33537.
I'll invert the PR stack.
💬 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
...