🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2354786373)
reACK 0b3ec8c59b2b6d598531cd4e688eb276e597c825
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2354786373)
reACK 0b3ec8c59b2b6d598531cd4e688eb276e597c825
👍 fanquake approved a pull request: "test: Remove 0.16.3 test from wallet_backwards_compatibility.py"
(https://github.com/bitcoin/bitcoin/pull/30920#pullrequestreview-2354794825)
ACK fae44c83da982095661b034bdd01afe8ac2fb0a6 - I agree that test seems to have past it's usefulness, and the fact that it otherwise causes intemittent issues is further reason to remove it.
(https://github.com/bitcoin/bitcoin/pull/30920#pullrequestreview-2354794825)
ACK fae44c83da982095661b034bdd01afe8ac2fb0a6 - I agree that test seems to have past it's usefulness, and the fact that it otherwise causes intemittent issues is further reason to remove it.
🚀 fanquake merged a pull request: "test: Remove 0.16.3 test from wallet_backwards_compatibility.py"
(https://github.com/bitcoin/bitcoin/pull/30920)
(https://github.com/bitcoin/bitcoin/pull/30920)
💬 maflcko commented on issue "Disallow building fuzz binary without `-DBUILD_FOR_FUZZING`":
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400143488)
> Historically, we used to build most binaries by default (including the fuzz binary) to ensure that compile failures are caught early by devs. With the switch to CMake this is no longer the case
Why would it no longer be the case with cmake? Are you proposing that every switches to a multi-config build?
> compile failures would be caught by the CI or other devs that regularly fuzz.
How would the CI catch compile or runtime failures on Windows or macOS, when the fuzz exe isn't compi
...
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400143488)
> Historically, we used to build most binaries by default (including the fuzz binary) to ensure that compile failures are caught early by devs. With the switch to CMake this is no longer the case
Why would it no longer be the case with cmake? Are you proposing that every switches to a multi-config build?
> compile failures would be caught by the CI or other devs that regularly fuzz.
How would the CI catch compile or runtime failures on Windows or macOS, when the fuzz exe isn't compi
...
💬 theuni commented on pull request "RFC: build: support for pre-compiled headers.":
(https://github.com/bitcoin/bitcoin/pull/31053#issuecomment-2400155446)
Huh, I guess that happens because pch shuffles around the include order.
The redundant decl indeed seems broken to me. Will PR a quick fix.
(https://github.com/bitcoin/bitcoin/pull/31053#issuecomment-2400155446)
Huh, I guess that happens because pch shuffles around the include order.
The redundant decl indeed seems broken to me. Will PR a quick fix.
👍 ryanofsky approved a pull request: "rpc: add optional blockhash to waitfornewblock"
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2354766572)
Code review ACK 13b867fb9b37fef3f14732904172e4ecba6fc973. I think implementation could be a ilttle simpler and more friendly though (see suggestion below)
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2354766572)
Code review ACK 13b867fb9b37fef3f14732904172e4ecba6fc973. I think implementation could be a ilttle simpler and more friendly though (see suggestion below)
💬 ryanofsky commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1792082432)
I think it would be good to simplify all of this to just:
```c++
// Use tip as reference block, unless a block hash was provided.
uint256 current_tip{request.params[1].isNull()
? miner.getTip().value_or(BlockRef{}).hash
: ParseHashV(request.params[1], "blockhash")};
BlockRef block{timeout > 0
? miner.waitTipChanged(current_tip, std::chrono::milliseconds(timeout))
: miner.waitTipChanged(current_tip)};
```
In addition to being simpler, th
...
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1792082432)
I think it would be good to simplify all of this to just:
```c++
// Use tip as reference block, unless a block hash was provided.
uint256 current_tip{request.params[1].isNull()
? miner.getTip().value_or(BlockRef{}).hash
: ParseHashV(request.params[1], "blockhash")};
BlockRef block{timeout > 0
? miner.waitTipChanged(current_tip, std::chrono::milliseconds(timeout))
: miner.waitTipChanged(current_tip)};
```
In addition to being simpler, th
...
💬 ryanofsky commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1792041394)
In commit "rpc: add optional blockhash to waitfornewblock" (13b867fb9b37fef3f14732904172e4ecba6fc973)
Maybe use phrasing from fa4c0750331f36121ba92bbc2f22c615b7934e52 and say "Method waits for the chain tip to differ from this." I think that's a little better because it describes behavior in general instead of describing a specific case where it returns immediately. I think it would also be better to call this `current_tip` instead of `blockhash` like the other variable, to be more descriptiv
...
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1792041394)
In commit "rpc: add optional blockhash to waitfornewblock" (13b867fb9b37fef3f14732904172e4ecba6fc973)
Maybe use phrasing from fa4c0750331f36121ba92bbc2f22c615b7934e52 and say "Method waits for the chain tip to differ from this." I think that's a little better because it describes behavior in general instead of describing a specific case where it returns immediately. I think it would also be better to call this `current_tip` instead of `blockhash` like the other variable, to be more descriptiv
...
📝 theuni opened a pull request: "refactor: include the proper header rather than forward-declaring RemovalReasonToString"
(https://github.com/bitcoin/bitcoin/pull/31058)
Trivial no-op fixup.
This was pointed out by #31053, which causes the include order to be shuffled around:
```
[21:49:26.130] /ci_container_base/src/validationinterface.cpp:22:13: error: redundant 'RemovalReasonToString' declaration [readability-redundant-declaration,-warnings-as-errors]
[21:49:26.130] 22 | std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept;
[21:49:26.130] | ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[21:4
...
(https://github.com/bitcoin/bitcoin/pull/31058)
Trivial no-op fixup.
This was pointed out by #31053, which causes the include order to be shuffled around:
```
[21:49:26.130] /ci_container_base/src/validationinterface.cpp:22:13: error: redundant 'RemovalReasonToString' declaration [readability-redundant-declaration,-warnings-as-errors]
[21:49:26.130] 22 | std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept;
[21:49:26.130] | ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[21:4
...
💬 dergoegge commented on issue "Disallow building fuzz binary without `-DBUILD_FOR_FUZZING`":
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400174059)
> Why would it no longer be the case with cmake? Are you proposing that every switches to a multi-config build?
What I meant, is that we no longer build the fuzz binary by default i.e. `-DBUILD_FOR_FUZZING=ON` or `-DBUILD_FUZZ_BINARY=ON` has to be added. Prior to cmake `./configure && make` would produce the fuzz binary, while now `cmake -B build/ && cmake --build build/` will not.
> How would the CI catch compile or runtime failures on Windows or macOS, when the fuzz exe isn't compiled and
...
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400174059)
> Why would it no longer be the case with cmake? Are you proposing that every switches to a multi-config build?
What I meant, is that we no longer build the fuzz binary by default i.e. `-DBUILD_FOR_FUZZING=ON` or `-DBUILD_FUZZ_BINARY=ON` has to be added. Prior to cmake `./configure && make` would produce the fuzz binary, while now `cmake -B build/ && cmake --build build/` will not.
> How would the CI catch compile or runtime failures on Windows or macOS, when the fuzz exe isn't compiled and
...
💬 maflcko commented on issue "Disallow building fuzz binary without `-DBUILD_FOR_FUZZING`":
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400183947)
> > Why would it no longer be the case with cmake? Are you proposing that every switches to a multi-config build?
>
> What I meant, is that we no longer build the fuzz binary by default i.e. `-DBUILD_FOR_FUZZING=ON` or `-DBUILD_FUZZ_BINARY=ON` has to be added. Prior to cmake `./configure && make` would produce the fuzz binary, while now `cmake -B build/ && cmake --build build/` will not.
I think the vanilla `cmake -B build` is for some imaginary end-user, not for developers. I imagine devs
...
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2400183947)
> > Why would it no longer be the case with cmake? Are you proposing that every switches to a multi-config build?
>
> What I meant, is that we no longer build the fuzz binary by default i.e. `-DBUILD_FOR_FUZZING=ON` or `-DBUILD_FUZZ_BINARY=ON` has to be added. Prior to cmake `./configure && make` would produce the fuzz binary, while now `cmake -B build/ && cmake --build build/` will not.
I think the vanilla `cmake -B build` is for some imaginary end-user, not for developers. I imagine devs
...
💬 maflcko commented on pull request "refactor: include the proper header rather than forward-declaring RemovalReasonToString":
(https://github.com/bitcoin/bitcoin/pull/31058#issuecomment-2400190661)
lgtm ACK ca2e4ba352cb0a3b5983c002b09ce15ebbf95177
This should be fine, because the circular dependency no longer exists since commit fad8c36aa9011c3f7b1183f8380577e16a2167a6
(https://github.com/bitcoin/bitcoin/pull/31058#issuecomment-2400190661)
lgtm ACK ca2e4ba352cb0a3b5983c002b09ce15ebbf95177
This should be fine, because the circular dependency no longer exists since commit fad8c36aa9011c3f7b1183f8380577e16a2167a6
🤔 naumenkogs reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2354761295)
Reviewed up to 3b78d8d5dc325bb32ea9ff167617e4ef80110301
Still learning all the stuff i missed with 1p1c, so my eyes were mostly focused on the refactoring correctness.
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2354761295)
Reviewed up to 3b78d8d5dc325bb32ea9ff167617e4ef80110301
Still learning all the stuff i missed with 1p1c, so my eyes were mostly focused on the refactoring correctness.
💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1792054515)
6fbb2463840f47f9f0632431e98a68f6747805ea
This variable is kinda synchronous with `m_peer_info`.
Would be unfortunate to decouple it accidentally in future code. Perhaps comment on it?
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1792054515)
6fbb2463840f47f9f0632431e98a68f6747805ea
This variable is kinda synchronous with `m_peer_info`.
Would be unfortunate to decouple it accidentally in future code. Perhaps comment on it?
💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1792037989)
6fbb2463840f47f9f0632431e98a68f6747805ea
Since this is a new variable... why not using a name more aligned with the description above? Otherwise it's hard too tell what this is about.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1792037989)
6fbb2463840f47f9f0632431e98a68f6747805ea
Since this is a new variable... why not using a name more aligned with the description above? Otherwise it's hard too tell what this is about.
💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1792083012)
1dd47af96cdc37706cd6d3f47892822e785b6fbe
What does the following mean: `p2p_inv=false` but `AlreadyHaveTx` is true?
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1792083012)
1dd47af96cdc37706cd6d3f47892822e785b6fbe
What does the following mean: `p2p_inv=false` but `AlreadyHaveTx` is true?
💬 hodlinator commented on pull request "test: Fix copy-paste in wallet/test/db_tests ostream operator":
(https://github.com/bitcoin/bitcoin/pull/31038#discussion_r1792120512)
Sorry, I wasn't fast enough.
(https://github.com/bitcoin/bitcoin/pull/31038#discussion_r1792120512)
Sorry, I wasn't fast enough.
🚀 ryanofsky merged a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967)
(https://github.com/bitcoin/bitcoin/pull/30967)
💬 hebasto commented on pull request "build: Bump minimum supported macOS to 12.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2400257076)
[Qt 6.8 LTS](https://doc.qt.io/qt-6.8/supported-platforms.html#macos) dropped support for macOS 11.
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2400257076)
[Qt 6.8 LTS](https://doc.qt.io/qt-6.8/supported-platforms.html#macos) dropped support for macOS 11.
💬 fanquake commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1792155583)
Think I agree with this as well.
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1792155583)
Think I agree with this as well.