👍 TheCharlatan approved a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2328340265)
ACK 1a332817665f77f55090fa166930fec0e5500727
Nice changes to the serialization code since my last review, re-using the existing concepts is great. I wonder if the Deserializable concept could just live in the `serialize.h` header. It seems like it could be generally useful?
My `clang-tidy` also wants to indent the `requires` expressions, but I don't think that is necessarily more readable.
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2328340265)
ACK 1a332817665f77f55090fa166930fec0e5500727
Nice changes to the serialization code since my last review, re-using the existing concepts is great. I wonder if the Deserializable concept could just live in the `serialize.h` header. It seems like it could be generally useful?
My `clang-tidy` also wants to indent the `requires` expressions, but I don't think that is necessarily more readable.
💬 stickies-v commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1775276096)
Thanks for looking into this further! I was also exploring the concepts route, but hadn't yet resolved all concerns.
1) Re naming: having `format` overloads with significantly differently behaviour (throwing vs non-throwing) didn't quite sit right with me, so I think your `format_raw` (or `format_throw`) suggestion is preferable, and I agree it makes more sense to rename the throwing variant instead of the non-throwing variant (although the downside is that it makes reviewing this PR less str
...
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1775276096)
Thanks for looking into this further! I was also exploring the concepts route, but hadn't yet resolved all concerns.
1) Re naming: having `format` overloads with significantly differently behaviour (throwing vs non-throwing) didn't quite sit right with me, so I think your `format_raw` (or `format_throw`) suggestion is preferable, and I agree it makes more sense to rename the throwing variant instead of the non-throwing variant (although the downside is that it makes reviewing this PR less str
...
⚠️ maflcko opened an issue: "win64-cross CI timeout after 2h"
(https://github.com/bitcoin/bitcoin/issues/30969)
https://cirrus-ci.com/task/6567392808009728?logs=ci#L2620
```
...
127/137 Test #131: spend_tests .......................... Passed 6.37 sec
Start 137: db_tests
128/137 Test #135: walletdb_tests ....................... Passed 3.02 sec
129/137 Test #137: db_tests ............................. Passed 2.91 sec
130/137 Test #136: walletload_tests ..................... Passed 5.63 sec
131/137 Test #133: wallet_tests ......................... Passed 14.11 sec
...
(https://github.com/bitcoin/bitcoin/issues/30969)
https://cirrus-ci.com/task/6567392808009728?logs=ci#L2620
```
...
127/137 Test #131: spend_tests .......................... Passed 6.37 sec
Start 137: db_tests
128/137 Test #135: walletdb_tests ....................... Passed 3.02 sec
129/137 Test #137: db_tests ............................. Passed 2.91 sec
130/137 Test #136: walletload_tests ..................... Passed 5.63 sec
131/137 Test #133: wallet_tests ......................... Passed 14.11 sec
...
💬 maflcko commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1775314729)
> I think c) is the preferable approach. It adds more core-specific LoC to tinyformat but I think it is the cleanest overall?
Yeah, probably. Maybe a large header `// === Everything below was added for Bitcoin Core ===` in the end and then appending the Bitcoin Core stuff would be cleaner than inlining every function into the existing header with a comment by itself.
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1775314729)
> I think c) is the preferable approach. It adds more core-specific LoC to tinyformat but I think it is the cleanest overall?
Yeah, probably. Maybe a large header `// === Everything below was added for Bitcoin Core ===` in the end and then appending the Bitcoin Core stuff would be cleaner than inlining every function into the existing header with a comment by itself.
💬 brunoerg commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1775342441)
Shouldn't we decrease the number of `inbounds_fanout_tx_relay`\`outbounds_fanout_tx_relay` by the number of peers we're going to fanout with ancestors?
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1775342441)
Shouldn't we decrease the number of `inbounds_fanout_tx_relay`\`outbounds_fanout_tx_relay` by the number of peers we're going to fanout with ancestors?
👍 ryanofsky approved a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2328408540)
Code review ACK fa61644768c3f6932860d1c5e2c6c0a38d5257e5.
Overall very nice changes, but I think the first commit fa5ac85d19a387c2e12fc0833368e1240a3dcf51 has a drawback because it adds a `NodeContext` argument to `RPCStop` when ideally RPC code should be agnostic to node initialization and not depend on node types. Would suggest following change to clean it up, which could be squashed into the first commit:
- ef271bc75e7bea8c82ce8667fb22e9b25dca5712 refactor: Split up NodeContext shutdown
...
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2328408540)
Code review ACK fa61644768c3f6932860d1c5e2c6c0a38d5257e5.
Overall very nice changes, but I think the first commit fa5ac85d19a387c2e12fc0833368e1240a3dcf51 has a drawback because it adds a `NodeContext` argument to `RPCStop` when ideally RPC code should be agnostic to node initialization and not depend on node types. Would suggest following change to clean it up, which could be squashed into the first commit:
- ef271bc75e7bea8c82ce8667fb22e9b25dca5712 refactor: Split up NodeContext shutdown
...
💬 ryanofsky commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775316336)
In commit "refactor: Replace g_genesis_wait_cv with m_tip_block_cv" (faaf888eb275cecc2d2803c5756afac633e0e2a4)
This code is waiting for `m_tip_block` to be non-null, but then code below will be assuming that `chainman.ActiveTip()` will be non-null. These are similar conditions but not exactly the same, so I think it would be good to add an assert below the if statement like `assert(WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip() != nullptr));` to make sure this code is func
...
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775316336)
In commit "refactor: Replace g_genesis_wait_cv with m_tip_block_cv" (faaf888eb275cecc2d2803c5756afac633e0e2a4)
This code is waiting for `m_tip_block` to be non-null, but then code below will be assuming that `chainman.ActiveTip()` will be non-null. These are similar conditions but not exactly the same, so I think it would be good to add an assert below the if statement like `assert(WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip() != nullptr));` to make sure this code is func
...
💬 ryanofsky commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775328397)
re: https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775211951
I'm not sure about the steady clock thing, but I think the reason this code is written to use a while loop is because it wants to wake up every 1000ms to check m_interrupt in case `m_interrupt` was signaled without `m_tip_block_cv` being signalled. Since I posted another change above to ensure that signalling `m_interrupt` will reliably signal `m_tip_block_cv`, I think this code could be changed to use a simple wait pred
...
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775328397)
re: https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775211951
I'm not sure about the steady clock thing, but I think the reason this code is written to use a while loop is because it wants to wake up every 1000ms to check m_interrupt in case `m_interrupt` was signaled without `m_tip_block_cv` being signalled. Since I posted another change above to ensure that signalling `m_interrupt` will reliably signal `m_tip_block_cv`, I think this code could be changed to use a simple wait pred
...
👍 TheCharlatan approved a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2328454931)
ACK fa61644768c3f6932860d1c5e2c6c0a38d5257e5
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2328454931)
ACK fa61644768c3f6932860d1c5e2c6c0a38d5257e5
💬 TheCharlatan commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775344259)
Nit: Could forward-declare `NodeContext` here instead.
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775344259)
Nit: Could forward-declare `NodeContext` here instead.
💬 maflcko commented on issue "win64-cross CI timeout after 2h":
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2374284122)
This is odd, because normally it should pass in less than 15 minutes:https://cirrus-ci.com/task/4920689327603712
Looks like `allocator_tests` never finished.
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2374284122)
This is odd, because normally it should pass in less than 15 minutes:https://cirrus-ci.com/task/4920689327603712
Looks like `allocator_tests` never finished.
💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775409498)
> These are similar conditions but not exactly the same
The `m_tip_block` condition implies the `Tip()` condition, because `SetTip()` happens before `blockTip()`.
An assert (`Assert`) is already present in the next commit, which was asked to be split up: https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775007062
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775409498)
> These are similar conditions but not exactly the same
The `m_tip_block` condition implies the `Tip()` condition, because `SetTip()` happens before `blockTip()`.
An assert (`Assert`) is already present in the next commit, which was asked to be split up: https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775007062
📝 maflcko converted_to_draft a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967)
`g_genesis_wait_cv` is similar to `m_tip_block_cv` but shuffling everything through a redundant `boost::signals2`.
So remove it, along with some other dead code, as well as minor fixups.
(https://github.com/bitcoin/bitcoin/pull/30967)
`g_genesis_wait_cv` is similar to `m_tip_block_cv` but shuffling everything through a redundant `boost::signals2`.
So remove it, along with some other dead code, as well as minor fixups.
👋 maflcko's pull request is ready for review: "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)
👍 ryanofsky approved a pull request: "init: Remove retry for loop"
(https://github.com/bitcoin/bitcoin/pull/30968#pullrequestreview-2328680000)
Code review ACK 705105de4f84f2ce3bb1d754c88c32349e01bb3b
Nice cleanup. Previous for loop was very confusing and new code is straightforward.
Not important, but I think it would be a little better if `InitAndLoadChainstate` simply returned `ChainstateLoadStatus` instead of `std::tuple</*may_retry*/ bool, /*error*/ bilingual_str>`. This way retry logic would be implemented in one function, not split across two functions. And it would avoid introducing a new way to represent success/interrupt
...
(https://github.com/bitcoin/bitcoin/pull/30968#pullrequestreview-2328680000)
Code review ACK 705105de4f84f2ce3bb1d754c88c32349e01bb3b
Nice cleanup. Previous for loop was very confusing and new code is straightforward.
Not important, but I think it would be a little better if `InitAndLoadChainstate` simply returned `ChainstateLoadStatus` instead of `std::tuple</*may_retry*/ bool, /*error*/ bilingual_str>`. This way retry logic would be implemented in one function, not split across two functions. And it would avoid introducing a new way to represent success/interrupt
...
💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775481467)
Thanks, but closing as no longer relevant. I'll follow-up with iwyu at some point, which should fix all of this either way.
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775481467)
Thanks, but closing as no longer relevant. I'll follow-up with iwyu at some point, which should fix all of this either way.
💬 itornaza commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2374471920)
> My `clang-tidy` also wants to indent the `requires` expressions, but I don't think that is necessarily more readable.
Checking with the C++ Core Guidelines and Stroustrup's books, while they do not mention anything about indenting require statements, they do indent it all through the guide like in this [minimal example](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rt-raise)
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2374471920)
> My `clang-tidy` also wants to indent the `requires` expressions, but I don't think that is necessarily more readable.
Checking with the C++ Core Guidelines and Stroustrup's books, while they do not mention anything about indenting require statements, they do indent it all through the guide like in this [minimal example](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rt-raise)
💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775489809)
> I'm not sure about the steady clock thing
I was referring to https://en.cppreference.com/w/cpp/thread/condition_variable/wait_until#Notes, but it can probably be dropped. I just wanted to mention that it may not be a refactor in a strict sense.
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1775489809)
> I'm not sure about the steady clock thing
I was referring to https://en.cppreference.com/w/cpp/thread/condition_variable/wait_until#Notes, but it can probably be dropped. I just wanted to mention that it may not be a refactor in a strict sense.
👍 itornaza approved a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2328717165)
ACK 1a332817665f77f55090fa166930fec0e5500727
Very clever rework to maximize code reuse while using already existing concepts. The code is now far easier to read. Built locally and all unit, and functional tests pass including the extended.
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2328717165)
ACK 1a332817665f77f55090fa166930fec0e5500727
Very clever rework to maximize code reuse while using already existing concepts. The code is now far easier to read. Built locally and all unit, and functional tests pass including the extended.
👍 ryanofsky approved a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2328742682)
Code review ACK fa7c2c9f18f5468baf81c04177b75e2b684a327a
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2328742682)
Code review ACK fa7c2c9f18f5468baf81c04177b75e2b684a327a