💬 ryanofsky commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517178327)
<!-- begin push-2 -->
Updated d0bd114e974500ad11c8ecac26d6606166ab7438 -> 0926479924f453c38b5e0ad069435fb3241b2389 ([`pr/klog.1`](https://github.com/ryanofsky/bitcoin/commits/pr/klog.1) -> [`pr/klog.2`](https://github.com/ryanofsky/bitcoin/commits/pr/klog.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/klog.1..pr/klog.2))<!-- end --> updating btck_logging_connection_create documentation
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517178327)
<!-- begin push-2 -->
Updated d0bd114e974500ad11c8ecac26d6606166ab7438 -> 0926479924f453c38b5e0ad069435fb3241b2389 ([`pr/klog.1`](https://github.com/ryanofsky/bitcoin/commits/pr/klog.1) -> [`pr/klog.2`](https://github.com/ryanofsky/bitcoin/commits/pr/klog.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/klog.1..pr/klog.2))<!-- end --> updating btck_logging_connection_create documentation
💬 ismaelsadeeq commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3517184939)
See https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514393534
That PR should be updated. This is the right direction IMO.
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3517184939)
See https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514393534
That PR should be updated. This is the right direction IMO.
💬 optout21 commented on pull request "refactor: C++20 operators":
(https://github.com/bitcoin/bitcoin/pull/33771#issuecomment-3517219391)
utACK 48840bfc2d7beeac0ddf56a3c26b243156ec8936
Straightforward refactoring resulting in code removal 😎
First commit removes `!=` operator where `==` exits. Second commit replaces 4 comparison operators with the new `<=>` spaceship operator.
My only concern is that competence with the new `<=>` operator may not be extensive across the developer community of this project, but hey, it's time to learn!
(https://github.com/bitcoin/bitcoin/pull/33771#issuecomment-3517219391)
utACK 48840bfc2d7beeac0ddf56a3c26b243156ec8936
Straightforward refactoring resulting in code removal 😎
First commit removes `!=` operator where `==` exits. Second commit replaces 4 comparison operators with the new `<=>` spaceship operator.
My only concern is that competence with the new `<=>` operator may not be extensive across the developer community of this project, but hey, it's time to learn!
✅ maflcko closed a pull request: "ci: Enable experimental kernel stuff in ASan task"
(https://github.com/bitcoin/bitcoin/pull/33845)
(https://github.com/bitcoin/bitcoin/pull/33845)
💬 maflcko commented on pull request "ci: Enable experimental kernel stuff in ASan task":
(https://github.com/bitcoin/bitcoin/pull/33845#issuecomment-3517219870)
Thanks for picking this up! Looks like a simple and small change in your pull request.
(https://github.com/bitcoin/bitcoin/pull/33845#issuecomment-3517219870)
Thanks for picking this up! Looks like a simple and small change in your pull request.
💬 optout21 commented on pull request "refactor: C++20 operators":
(https://github.com/bitcoin/bitcoin/pull/33771#issuecomment-3517222722)
I wonder whether it would it be possible check for superfluous comparison operator implementations in linting?
(https://github.com/bitcoin/bitcoin/pull/33771#issuecomment-3517222722)
I wonder whether it would it be possible check for superfluous comparison operator implementations in linting?
💬 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.