💬 fanquake commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#discussion_r2522793220)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/33537#discussion_r2522793220)
Fixed.
💬 stickies-v commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2522796090)
_response to https://github.com/bitcoin/bitcoin/pull/33855#issuecomment-3526237850_
@alexanderwiederin opening this thread for easier focus. Are you suggesting we 1) implement `btck_block_tree_entry_equals` with a hash equality check, or 2) to not expose `btck_block_tree_entry_equals` at all and let the user define it however they want (such as using hash equality), or 3) something else?
If 1), I don't see the benefit? Pointer comparisons are faster, and are conceptually correct, and what
...
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2522796090)
_response to https://github.com/bitcoin/bitcoin/pull/33855#issuecomment-3526237850_
@alexanderwiederin opening this thread for easier focus. Are you suggesting we 1) implement `btck_block_tree_entry_equals` with a hash equality check, or 2) to not expose `btck_block_tree_entry_equals` at all and let the user define it however they want (such as using hash equality), or 3) something else?
If 1), I don't see the benefit? Pointer comparisons are faster, and are conceptually correct, and what
...
💬 maflcko commented on pull request "ci: Lint follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33776#issuecomment-3527196104)
> It's a shame that in this PR we have to have the annotation printed many times, one per job, as I think it might drown out other notices.
Agree, but I think we can use warning notices to bubble up more important ones.
> Without knowing exactly how this machine readable annotation is being consumed or experimenting more, I'm not aware of an alternative?
It can be used via the API, e.g. https://github.com/maflcko/DrahtBot/blob/6c91378116516ca5974d78a4e0c5c495b38acf95/webhook_feature
...
(https://github.com/bitcoin/bitcoin/pull/33776#issuecomment-3527196104)
> It's a shame that in this PR we have to have the annotation printed many times, one per job, as I think it might drown out other notices.
Agree, but I think we can use warning notices to bubble up more important ones.
> Without knowing exactly how this machine readable annotation is being consumed or experimenting more, I'm not aware of an alternative?
It can be used via the API, e.g. https://github.com/maflcko/DrahtBot/blob/6c91378116516ca5974d78a4e0c5c495b38acf95/webhook_feature
...
💬 vasild commented on pull request "net_processing: reorder the code that handles the VERSION message":
(https://github.com/bitcoin/bitcoin/pull/33792#issuecomment-3527310703)
Good question, I guess it is a bit obscure. Let me elaborate. So, when we receive the `VERSION` message, its handling (inside the `if (msg_type == NetMsgType::VERSION) {`) currently goes like this:
```
if (...)
A
if (...)
MakeAndPushMessage()
if (...)
B
if (...)
MakeAndPushMessage()
if (...)
some stuff that better be avoided for private broadcast connections
```
For private broadcast we don't want to push any extra messages. To do that I took the approach to
...
(https://github.com/bitcoin/bitcoin/pull/33792#issuecomment-3527310703)
Good question, I guess it is a bit obscure. Let me elaborate. So, when we receive the `VERSION` message, its handling (inside the `if (msg_type == NetMsgType::VERSION) {`) currently goes like this:
```
if (...)
A
if (...)
MakeAndPushMessage()
if (...)
B
if (...)
MakeAndPushMessage()
if (...)
some stuff that better be avoided for private broadcast connections
```
For private broadcast we don't want to push any extra messages. To do that I took the approach to
...
💬 vasild commented on pull request "net_processing: reorder the code that handles the VERSION message":
(https://github.com/bitcoin/bitcoin/pull/33792#issuecomment-3527377529)
> What’s the rationale for moving `m_addrman.Good(pfrom.addr)` earlier ?
Hmm, wait! `m_addrman.Good()` need not be earlier, it is one of the "some stuff that better be avoided". Then this can be simplified? :bulb:
(https://github.com/bitcoin/bitcoin/pull/33792#issuecomment-3527377529)
> What’s the rationale for moving `m_addrman.Good(pfrom.addr)` earlier ?
Hmm, wait! `m_addrman.Good()` need not be earlier, it is one of the "some stuff that better be avoided". Then this can be simplified? :bulb:
💬 ajtowns commented on pull request "txgraph: drop move assignment operator":
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3527384125)
ACK aef40b93cf057d2a27d61881b0858d491206bcd3 -- matches what I was thinking
Adding some assertions in `Ref::Ref(Ref&&)` triggers when I run the fuzz binary over some txgraph corpus data I generated previously, so https://github.com/bitcoin/bitcoin/pull/33862#discussion_r2519205585 doesn't seem like a real problem.
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3527384125)
ACK aef40b93cf057d2a27d61881b0858d491206bcd3 -- matches what I was thinking
Adding some assertions in `Ref::Ref(Ref&&)` triggers when I run the fuzz binary over some txgraph corpus data I generated previously, so https://github.com/bitcoin/bitcoin/pull/33862#discussion_r2519205585 doesn't seem like a real problem.
👍 l0rinc approved a pull request: "net_processing: rename RelayTransaction to better describe what it does"
(https://github.com/bitcoin/bitcoin/pull/33565#pullrequestreview-3459390175)
I don't want to be dismissive of @ajtowns' claims. I agree they're all valid, and please allow me to push back respectfully.
> bikeshedding about the names of functions is not a good use of anyone's time here.
I've been tricked many times by misnamed methods. If you think the old name is good, or the new name is worse, or the new comment isn't adding anything, I can empathize with that. But nobody is forcing us to review this. I personally do it because I want some progress with the origin
...
(https://github.com/bitcoin/bitcoin/pull/33565#pullrequestreview-3459390175)
I don't want to be dismissive of @ajtowns' claims. I agree they're all valid, and please allow me to push back respectfully.
> bikeshedding about the names of functions is not a good use of anyone's time here.
I've been tricked many times by misnamed methods. If you think the old name is good, or the new name is worse, or the new comment isn't adding anything, I can empathize with that. But nobody is forcing us to review this. I personally do it because I want some progress with the origin
...
💬 l0rinc commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2523145922)
the enum name aligns more closely with the method name 👍
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2523145922)
the enum name aligns more closely with the method name 👍
💬 stickies-v commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523160503)
I can't seem to reproduce that, e.g. these tests all pass for me, indicating that an empty or null path resolves to `fs::path()`, which then expands to `fs::current_path()`? How did you get that fatal error?
```cpp
BOOST_CHECK_EQUAL(fs::PathFromString(""), fs::path());
BOOST_CHECK_EQUAL(fs::PathFromString({nullptr, 0}), fs::path());
BOOST_CHECK_EQUAL(fs::canonical(fs::path()), fs::current_path());
```
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523160503)
I can't seem to reproduce that, e.g. these tests all pass for me, indicating that an empty or null path resolves to `fs::path()`, which then expands to `fs::current_path()`? How did you get that fatal error?
```cpp
BOOST_CHECK_EQUAL(fs::PathFromString(""), fs::path());
BOOST_CHECK_EQUAL(fs::PathFromString({nullptr, 0}), fs::path());
BOOST_CHECK_EQUAL(fs::canonical(fs::path()), fs::current_path());
```
💬 maflcko commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523224076)
> How did you get that fatal error?
It is hidden in the error message, but it says "cannot make absolute path".
Did you actually call the full kernel function here, including:
```cpp
fs::path abs_data_dir{fs::absolute(fs::PathFromString({data_dir, data_dir_len}))};
fs::create_directories(abs_data_dir);
```
In any case, even if it doesn't crash, I wonder what the valid meaning is here, of passing an empty string?
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523224076)
> How did you get that fatal error?
It is hidden in the error message, but it says "cannot make absolute path".
Did you actually call the full kernel function here, including:
```cpp
fs::path abs_data_dir{fs::absolute(fs::PathFromString({data_dir, data_dir_len}))};
fs::create_directories(abs_data_dir);
```
In any case, even if it doesn't crash, I wonder what the valid meaning is here, of passing an empty string?
💬 yancyribbens commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523230856)
In what situation would `data_dir == nullptr` and yet `data_dir_len` does not equal zero? In other-words, if data_dir is null and data_dir_len is `1` then continue?
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523230856)
In what situation would `data_dir == nullptr` and yet `data_dir_len` does not equal zero? In other-words, if data_dir is null and data_dir_len is `1` then continue?
💬 ryanofsky commented on pull request "scripted-diff: fix leftover references to `policy/fees.h`":
(https://github.com/bitcoin/bitcoin/pull/33864#issuecomment-3527726460)
Thanks for the cleanup!
(https://github.com/bitcoin/bitcoin/pull/33864#issuecomment-3527726460)
Thanks for the cleanup!
💬 fanquake commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3527955912)
Guix Build (x86_64 & aarch64):
```bash
fe9e088e3481013bf59dfc500803b415bf29a28cb8721974040e1ea7d0a75770 guix-build-96963b888e5a/output/aarch64-linux-gnu/SHA256SUMS.part
7e070397b6bace67e0960c2e9838717c77117571eaff88aaad9e6c03d24a9400 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarch64-linux-gnu-debug.tar.gz
9979dbb9896915bf99c9d23accccb5cab8f6b81de17c51d03c328763270f14f3 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarch64-linux-gnu.tar.g
...
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3527955912)
Guix Build (x86_64 & aarch64):
```bash
fe9e088e3481013bf59dfc500803b415bf29a28cb8721974040e1ea7d0a75770 guix-build-96963b888e5a/output/aarch64-linux-gnu/SHA256SUMS.part
7e070397b6bace67e0960c2e9838717c77117571eaff88aaad9e6c03d24a9400 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarch64-linux-gnu-debug.tar.gz
9979dbb9896915bf99c9d23accccb5cab8f6b81de17c51d03c328763270f14f3 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarch64-linux-gnu.tar.g
...
💬 maflcko commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3527984072)
review ACK 6d2c8ea9dbd77c71051935b5ab59224487509559 🔰
<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_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 6d2c8ea9dbd7
...
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3527984072)
review ACK 6d2c8ea9dbd77c71051935b5ab59224487509559 🔰
<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_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 6d2c8ea9dbd7
...
✅ TheCharlatan closed a pull request: "kernel: Remove dependency on clientversion"
(https://github.com/bitcoin/bitcoin/pull/32543)
(https://github.com/bitcoin/bitcoin/pull/32543)
💬 TheCharlatan commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-3528096559)
Closing this. Will pick it up again as part of a larger directional push to handle versioning in the kernel.
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-3528096559)
Closing this. Will pick it up again as part of a larger directional push to handle versioning in the kernel.
💬 brunoerg commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2523702039)
Nice catch, we can simply check `hd_chain.nVersion` instead of having `auto hd_chain_version{legacy_data.GetHDChain().nVersion};`. We could also add an assert to ensure these two values are the same.
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2523702039)
Nice catch, we can simply check `hd_chain.nVersion` instead of having `auto hd_chain_version{legacy_data.GetHDChain().nVersion};`. We could also add an assert to ensure these two values are the same.
💬 brunoerg commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2523711918)
No null check because we always expect it to succeed given how the harness is built, but will add a check just to avoid UB.
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2523711918)
No null check because we always expect it to succeed given how the harness is built, but will add a check just to avoid UB.
💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2523758447)
logging the block hash here is not as straightforward as it looks, because we would have to know which of the headers failed, and `ProcessNewBlockHeaders` doesn't return that info:
So I think we would either have to change the interface of `ProcessNewBlockHeaders`, or we would have to add a loop over `headers` (`CBlockHeader`) here, lookup each header in our block index and check whether it's `BLOCK_FAILED_MASK`.
I'm on vacation this week, will look into it more next week.
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2523758447)
logging the block hash here is not as straightforward as it looks, because we would have to know which of the headers failed, and `ProcessNewBlockHeaders` doesn't return that info:
So I think we would either have to change the interface of `ProcessNewBlockHeaders`, or we would have to add a loop over `headers` (`CBlockHeader`) here, lookup each header in our block index and check whether it's `BLOCK_FAILED_MASK`.
I'm on vacation this week, will look into it more next week.
📝 andrewtoth opened a pull request: "Remove incorrect lifetimebounds"
(https://github.com/bitcoin/bitcoin/pull/33870)
(https://github.com/bitcoin/bitcoin/pull/33870)