💬 mzumsande commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1759034089)
I agree that it shouldn't be possible to have undo data but no block data.
On the other hand, callers of this function may not even be interested in the block data, and I'm not sure if a RPC call is the right place to check for this inconsistency. At least it's checked in `CheckBlockIndex()` [here](https://github.com/bitcoin/bitcoin/blob/06a9f7789e359131abad2489a221888f711bdc3b/src/validation.cpp#L5326), so I think I'll leave this as is for now.
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1759034089)
I agree that it shouldn't be possible to have undo data but no block data.
On the other hand, callers of this function may not even be interested in the block data, and I'm not sure if a RPC call is the right place to check for this inconsistency. At least it's checked in `CheckBlockIndex()` [here](https://github.com/bitcoin/bitcoin/blob/06a9f7789e359131abad2489a221888f711bdc3b/src/validation.cpp#L5326), so I think I'll leave this as is for now.
💬 mzumsande commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1759034840)
fixed
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1759034840)
fixed
💬 mzumsande commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#issuecomment-2349173516)
[a5df908 ](https://github.com/bitcoin/bitcoin/commit/a5df908d981138f066f5f963a1813d453483814a)to [6a1aa51](https://github.com/bitcoin/bitcoin/commit/6a1aa510e31e8b77793341473aa5afc9d023a6e3): Fixed typos.
(https://github.com/bitcoin/bitcoin/pull/30410#issuecomment-2349173516)
[a5df908 ](https://github.com/bitcoin/bitcoin/commit/a5df908d981138f066f5f963a1813d453483814a)to [6a1aa51](https://github.com/bitcoin/bitcoin/commit/6a1aa510e31e8b77793341473aa5afc9d023a6e3): Fixed typos.
👍 pablomartin4btc approved a pull request: "doc: unit test runner help fixup and improvements"
(https://github.com/bitcoin/bitcoin/pull/30890#pullrequestreview-2303389366)
re-ACK 282f0e92559da23e356504a564a0322b9888e50b
Thanks!
(https://github.com/bitcoin/bitcoin/pull/30890#pullrequestreview-2303389366)
re-ACK 282f0e92559da23e356504a564a0322b9888e50b
Thanks!
💬 sr-gi commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1759046001)
We were not checking it before, nor are we doing so later when emplacing it in `m_peer_map` either, so I'd lean towards no for consistency, but I don't have a strong opinion about it
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1759046001)
We were not checking it before, nor are we doing so later when emplacing it in `m_peer_map` either, so I'd lean towards no for consistency, but I don't have a strong opinion about it
👍 tdb3 approved a pull request: "doc: unit test runner help fixup and improvements"
(https://github.com/bitcoin/bitcoin/pull/30890#pullrequestreview-2303400654)
re ACK 282f0e92559da23e356504a564a0322b9888e50b
Latest change is moving the line about `--list-content`
(https://github.com/bitcoin/bitcoin/pull/30890#pullrequestreview-2303400654)
re ACK 282f0e92559da23e356504a564a0322b9888e50b
Latest change is moving the line about `--list-content`
💬 fanquake commented on issue "cmake: Installed static kernel library is unusable":
(https://github.com/bitcoin/bitcoin/issues/30801#issuecomment-2349209374)
@TheCharlatan what is the status of this now?
(https://github.com/bitcoin/bitcoin/issues/30801#issuecomment-2349209374)
@TheCharlatan what is the status of this now?
👍 andrewtoth approved a pull request: "rpc, rest: Improve block rpc error handling, check header before attempting to read block data."
(https://github.com/bitcoin/bitcoin/pull/30410#pullrequestreview-2303417074)
re-ACK 6a1aa510e31e8b77793341473aa5afc9d023a6e3
(https://github.com/bitcoin/bitcoin/pull/30410#pullrequestreview-2303417074)
re-ACK 6a1aa510e31e8b77793341473aa5afc9d023a6e3
💬 andrewtoth commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1759063676)
nit: For consistency, we might want to update this function too to take `hash` as the first parameter, even if not needed.
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1759063676)
nit: For consistency, we might want to update this function too to take `hash` as the first parameter, even if not needed.
💬 andrewtoth commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1759057303)
nit: why remove the extra context that we are testing with verbosity 2 and 3 specifically?
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1759057303)
nit: why remove the extra context that we are testing with verbosity 2 and 3 specifically?
👍 itornaza approved a pull request: "Have createNewBlock() return a BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/30440#pullrequestreview-2303431473)
Code review ACK a93c171faa7b4426823466e972c8f24260918bbf
Offering an option for the Template Provider to get block header data as soon as possible is a great addition.
Code changes look straight forward. Also, built the pr and successfully run all tests on it locally.
(https://github.com/bitcoin/bitcoin/pull/30440#pullrequestreview-2303431473)
Code review ACK a93c171faa7b4426823466e972c8f24260918bbf
Offering an option for the Template Provider to get block header data as soon as possible is a great addition.
Code changes look straight forward. Also, built the pr and successfully run all tests on it locally.
💬 fanquake commented on pull request "guix: Pointer Authentication and Branch Target Identification for aarch64 Linux":
(https://github.com/bitcoin/bitcoin/pull/24123#issuecomment-2349221011)
Rebased for #30433, and updated to add an export allowance for `__libc_single_threaded`.
(https://github.com/bitcoin/bitcoin/pull/24123#issuecomment-2349221011)
Rebased for #30433, and updated to add an export allowance for `__libc_single_threaded`.
💬 maflcko commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#issuecomment-2349225738)
ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5 🗞
<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: ACK 07f4cebe5780f1039541d989e6
...
(https://github.com/bitcoin/bitcoin/pull/30233#issuecomment-2349225738)
ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5 🗞
<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: ACK 07f4cebe5780f1039541d989e6
...
💬 rkrux commented on issue "28.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2349229940)
@monlovesmango Thanks for sharing your feedback. I've gone through them and these points are correct, I've updated the guide.
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2349229940)
@monlovesmango Thanks for sharing your feedback. I've gone through them and these points are correct, I've updated the guide.
💬 achow101 commented on pull request "[28.x] Further backports and rc2":
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2349234397)
> @achow101
>
> Could you please add [bitcoin-core/gui#835](https://github.com/bitcoin-core/gui/pull/835) as well?
Done
(https://github.com/bitcoin/bitcoin/pull/30827#issuecomment-2349234397)
> @achow101
>
> Could you please add [bitcoin-core/gui#835](https://github.com/bitcoin-core/gui/pull/835) as well?
Done
💬 sdaftuar commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2349244010)
I have been gathering more data to try to measure the effect of the improvements in this PR. I decided to take my cluster mempool branch (#28676), combined with my simulation environment for playing back historical data, and measure the effect of a series of 4 commits from this PR on the number of clusters encountered in 2023 that would NOT have been optimally linearized, for each of 5 different values of the maximum iterations we'd permit when linearizing.
I think plotted these values on
...
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2349244010)
I have been gathering more data to try to measure the effect of the improvements in this PR. I decided to take my cluster mempool branch (#28676), combined with my simulation environment for playing back historical data, and measure the effect of a series of 4 commits from this PR on the number of clusters encountered in 2023 that would NOT have been optimally linearized, for each of 5 different values of the maximum iterations we'd permit when linearizing.
I think plotted these values on
...
💬 mzumsande commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2349247257)
> Ping also @mzumsande, given you https://github.com/bitcoin/bitcoin/pull/28043#pullrequestreview-1846077000 to this approach last time.
No reservations with the current approach. Concept ACK, I haven't reviewed the actual fuzz target in detail (and don't know if I would get to that in time, considering this has already multiple acks).
Would it make sense to additionally have some belt-and-suspenders check somewhere at the start of bitcoind that `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` i
...
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2349247257)
> Ping also @mzumsande, given you https://github.com/bitcoin/bitcoin/pull/28043#pullrequestreview-1846077000 to this approach last time.
No reservations with the current approach. Concept ACK, I haven't reviewed the actual fuzz target in detail (and don't know if I would get to that in time, considering this has already multiple acks).
Would it make sense to additionally have some belt-and-suspenders check somewhere at the start of bitcoind that `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` i
...
✅ TheCharlatan closed an issue: "cmake: Installed static kernel library is unusable"
(https://github.com/bitcoin/bitcoin/issues/30801)
(https://github.com/bitcoin/bitcoin/issues/30801)
💬 TheCharlatan commented on issue "cmake: Installed static kernel library is unusable":
(https://github.com/bitcoin/bitcoin/issues/30801#issuecomment-2349255516)
> @TheCharlatan what is the status of this now?
Just tested that this works with the kernel rust crate now for statically linking the library into a rust binary. So I think we are done here.
(https://github.com/bitcoin/bitcoin/issues/30801#issuecomment-2349255516)
> @TheCharlatan what is the status of this now?
Just tested that this works with the kernel rust crate now for statically linking the library into a rust binary. So I think we are done here.
💬 fanquake commented on pull request "code style: update .editorconfig file":
(https://github.com/bitcoin/bitcoin/pull/30877#discussion_r1759098222)
@theStack did you want to push to take this, given this file is rarely touched?
(https://github.com/bitcoin/bitcoin/pull/30877#discussion_r1759098222)
@theStack did you want to push to take this, given this file is rarely touched?