🚀 fanquake merged a pull request: "test: Treat exclude list warning as failure in CI"
(https://github.com/bitcoin/bitcoin/pull/31018)
(https://github.com/bitcoin/bitcoin/pull/31018)
💬 fanquake commented on pull request "Add -pausebackgroundsync startup option":
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2400031610)
I agree with the other commentors here. I don't think we should be trying to extend/re-use a user-facing feature for development purposes, especially given the existence of more accurate/repeatable ways of achieving the same thing. Adding this option for the other uses-cases described just further murkies the function of/complicates this feature.
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2400031610)
I agree with the other commentors here. I don't think we should be trying to extend/re-use a user-facing feature for development purposes, especially given the existence of more accurate/repeatable ways of achieving the same thing. Adding this option for the other uses-cases described just further murkies the function of/complicates this feature.
🚀 fanquake merged a pull request: "ci: Add missing -DWERROR=ON to test-each-commit"
(https://github.com/bitcoin/bitcoin/pull/31045)
(https://github.com/bitcoin/bitcoin/pull/31045)
💬 joe-bp commented on issue "Embedded ASMap data Tracking Issue":
(https://github.com/bitcoin/bitcoin/issues/28794#issuecomment-2400037617)
> > Is there no way to query AS Maps directly from the BGP tables?
>
> Who's BGP table? Can you be a bit more specific what you are thinking of in terms of architecture and trust assumptions?
Hi Fabian, I've been talking to Gary about this a bit. I submitted an issue here: https://github.com/asmap/asmap-data/issues/17
I'd like to propose a way to pull in the actual real-time BGP routing table and use that data for asmap. Multiple people/orgs can do this independently and get the data
...
(https://github.com/bitcoin/bitcoin/issues/28794#issuecomment-2400037617)
> > Is there no way to query AS Maps directly from the BGP tables?
>
> Who's BGP table? Can you be a bit more specific what you are thinking of in terms of architecture and trust assumptions?
Hi Fabian, I've been talking to Gary about this a bit. I submitted an issue here: https://github.com/asmap/asmap-data/issues/17
I'd like to propose a way to pull in the actual real-time BGP routing table and use that data for asmap. Multiple people/orgs can do this independently and get the data
...
✅ fanquake closed a pull request: "doc: clarify 'filename' argument in 'loadwallet' RPC"
(https://github.com/bitcoin/bitcoin/pull/31031)
(https://github.com/bitcoin/bitcoin/pull/31031)
💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2400044072)
Force-pushed to rebase addressing a merge conflict, preserve `tfm::format_error` throwing behaviour for `DEBUG` builds to improve visibility into programming errors before they manifest, and [improve an error string](https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2389355374).
---
Thank you for your thoughtful review @ryanofsky. tl;dr I share your concerns, but I think I have a different opinion on the trade-offs made.
I think everyone here is in agreement that compile-time c
...
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2400044072)
Force-pushed to rebase addressing a merge conflict, preserve `tfm::format_error` throwing behaviour for `DEBUG` builds to improve visibility into programming errors before they manifest, and [improve an error string](https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2389355374).
---
Thank you for your thoughtful review @ryanofsky. tl;dr I share your concerns, but I think I have a different opinion on the trade-offs made.
I think everyone here is in agreement that compile-time c
...
🤔 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?