👍 ryanofsky approved a pull request: "build: Introduce internal kernel library"
(https://github.com/bitcoin/bitcoin/pull/28690#pullrequestreview-1768762282)
Code review ACK 34970bde23dd87fc0fea5a1970880761f7184774.
Main change since last review is no longer moving the spanparsing.h file here, to avoid making util depend on common. I have since opened a separate PR #29015 to remove spanparsing.h functions from util by splitting it up.
(https://github.com/bitcoin/bitcoin/pull/28690#pullrequestreview-1768762282)
Code review ACK 34970bde23dd87fc0fea5a1970880761f7184774.
Main change since last review is no longer moving the spanparsing.h file here, to avoid making util depend on common. I have since opened a separate PR #29015 to remove spanparsing.h functions from util by splitting it up.
🤔 mzumsande reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1768762854)
ACK f479a99cc92c7966266804e8c33338cebbc44fbc
I have reviewed the code. I did some testing / gained more confidence that it works well by having a [branch](https://github.com/mzumsande/bitcoin/tree/tmp_bip324_fixalltests) where, with some adjustments, the python v2 p2p module is used for the entire functional test suite.
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1768762854)
ACK f479a99cc92c7966266804e8c33338cebbc44fbc
I have reviewed the code. I did some testing / gained more confidence that it works well by having a [branch](https://github.com/mzumsande/bitcoin/tree/tmp_bip324_fixalltests) where, with some adjustments, the python v2 p2p module is used for the entire functional test suite.
💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1418061039)
One thing to consider is that now we have two locks for p2p, we need to worry about deadlocks.
There is an implied lock order, `on_message` locks `p2p_lock`, and can call messages (e.g. `on_inv`) that will call `send_message` and lock `_send_lock`. So we must make sure never to take `p2p_lock` after `_send_lock`. I don't think that this is currently possible, but maybe we should add a comment about this.
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1418061039)
One thing to consider is that now we have two locks for p2p, we need to worry about deadlocks.
There is an implied lock order, `on_message` locks `p2p_lock`, and can call messages (e.g. `on_inv`) that will call `send_message` and lock `_send_lock`. So we must make sure never to take `p2p_lock` after `_send_lock`. I don't think that this is currently possible, but maybe we should add a comment about this.
💬 TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1418067302)
Re https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1417977573
> An alternative would be to just remove it from the util library and keep it only in consensus?
These functions are used by components of `bitcoin-cli`. The `bitcoin-cli` binary should not have to rely on the consensus library, so I don't think that is an alternative.
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1418067302)
Re https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1417977573
> An alternative would be to just remove it from the util library and keep it only in consensus?
These functions are used by components of `bitcoin-cli`. The `bitcoin-cli` binary should not have to rely on the consensus library, so I don't think that is an alternative.
💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1418077030)
> Thanks for the rationale, I indeed did not think of the future IPC delay. If we only start processing events after `app.exec()`, why not move this to just before `registerShutdownBlockReason`? Looking at the history it looks to me like this was put where it is to be in the general vicinity of the `installEvent*` calls.
I can make this change if you still think it's a good idea after this, but I think it's reasonable to want to keep the installEvent calls together. I also didn't want to just
...
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1418077030)
> Thanks for the rationale, I indeed did not think of the future IPC delay. If we only start processing events after `app.exec()`, why not move this to just before `registerShutdownBlockReason`? Looking at the history it looks to me like this was put where it is to be in the general vicinity of the `installEvent*` calls.
I can make this change if you still think it's a good idea after this, but I think it's reasonable to want to keep the installEvent calls together. I also didn't want to just
...
💬 sipsorcery commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1843853088)
Maybe something in txmempool.h that the msvc compiler doesn't like?
```
D:\a\bitcoin\bitcoin\src\txmempool.h(457,60): error C2143: syntax error: missing ')' before 'public' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_qt\libbitcoin_qt.vcxproj]
D:\a\bitcoin\bitcoin\src\txmempool.h(457,60): error C2143: syntax error: missing ';' before 'public' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_qt\libbitcoin_qt.vcxproj]
D:\a\bitcoin\bitcoin\src\txmempool.h(457,67): error C2059: syntax error: ')' [D:
...
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1843853088)
Maybe something in txmempool.h that the msvc compiler doesn't like?
```
D:\a\bitcoin\bitcoin\src\txmempool.h(457,60): error C2143: syntax error: missing ')' before 'public' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_qt\libbitcoin_qt.vcxproj]
D:\a\bitcoin\bitcoin\src\txmempool.h(457,60): error C2143: syntax error: missing ';' before 'public' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_qt\libbitcoin_qt.vcxproj]
D:\a\bitcoin\bitcoin\src\txmempool.h(457,67): error C2059: syntax error: ')' [D:
...
💬 ajtowns commented on pull request "wip: Split fuzz binary":
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1843941916)
> The cumulative size of the individual binaries (compiled with LTO) is 3.6GB vs 14GB when search and replacing `std::getenv("FUZZ")` (like we do in oss-fuzz).
What does this mean? I'm seeing file sizes of 448MB (with fuzzing enabled) and 146MB (with a normal build) for test/fuzz/fuzz.
This seems like it's reverting #20560?
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1843941916)
> The cumulative size of the individual binaries (compiled with LTO) is 3.6GB vs 14GB when search and replacing `std::getenv("FUZZ")` (like we do in oss-fuzz).
What does this mean? I'm seeing file sizes of 448MB (with fuzzing enabled) and 146MB (with a normal build) for test/fuzz/fuzz.
This seems like it's reverting #20560?
💬 kevkevinpal commented on pull request "RPC: add new `listmempooltransactions`":
(https://github.com/bitcoin/bitcoin/pull/29016#discussion_r1418224065)
looks like the tidy ci is getting upset here
```
rpc/mempool.cpp:462:82: error: argument name 'inclue_hex' in comment does not match parameter name 'include_hex' [bugprone-argument-comment,-warnings-as-errors]
462 | TxToUniv(e.GetTx(), /*block_hash=*/uint256::ZERO, /*entry=*/txentry, /*inclue_hex=*/false);
| ^~~~~~~~~~~~~~~
|
...
(https://github.com/bitcoin/bitcoin/pull/29016#discussion_r1418224065)
looks like the tidy ci is getting upset here
```
rpc/mempool.cpp:462:82: error: argument name 'inclue_hex' in comment does not match parameter name 'include_hex' [bugprone-argument-comment,-warnings-as-errors]
462 | TxToUniv(e.GetTx(), /*block_hash=*/uint256::ZERO, /*entry=*/txentry, /*inclue_hex=*/false);
| ^~~~~~~~~~~~~~~
|
...
💬 ajtowns commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1844180519)
> Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications.
Is the idea here that `util` should be for generic code, and that these bits and pieces (like fees and error) are specific bitcoin features that are only relevant to wallet/p2p, so aren't generic?
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1844180519)
> Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications.
Is the idea here that `util` should be for generic code, and that these bits and pieces (like fees and error) are specific bitcoin features that are only relevant to wallet/p2p, so aren't generic?
💬 sdaftuar commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1418261403)
Is there a reason that we are invoking `GetSortedDepthAndScore()` here, ie do any of the callers rely on a particular sort order?
(I'm planning to eliminate this function in #28676, so just want to understand if this behavior needs to be preserved for some reason.)
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1418261403)
Is there a reason that we are invoking `GetSortedDepthAndScore()` here, ie do any of the callers rely on a particular sort order?
(I'm planning to eliminate this function in #28676, so just want to understand if this behavior needs to be preserved for some reason.)
🤔 pablomartin4btc reviewed a pull request: "RPC: add new `listmempooltransactions`"
(https://github.com/bitcoin/bitcoin/pull/29016#pullrequestreview-1769056893)
Also, [missing](https://github.com/bitcoin/bitcoin/pull/29016/checks?check_run_id=19389652409) test coverage for the new command:
```
Uncovered RPC commands:
- listmempooltransactions
```
(https://github.com/bitcoin/bitcoin/pull/29016#pullrequestreview-1769056893)
Also, [missing](https://github.com/bitcoin/bitcoin/pull/29016/checks?check_run_id=19389652409) test coverage for the new command:
```
Uncovered RPC commands:
- listmempooltransactions
```
💬 stratospher commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418333521)
> I wonder if this can be generalized to pass test-only options (along with a helper HasTestOption, similar to IsDeprecatedRPCEnabled?
what would a function like `HasTestOption` do though? we could introduce it here to check if we're using a known test option("relay", "addrman"). even if we use unknown test options, we don't need to throw an error right.
> In any case, (default: %s)", "relay" is wrong, no?
done.
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418333521)
> I wonder if this can be generalized to pass test-only options (along with a helper HasTestOption, similar to IsDeprecatedRPCEnabled?
what would a function like `HasTestOption` do though? we could introduce it here to check if we're using a known test option("relay", "addrman"). even if we use unknown test options, we don't need to throw an error right.
> In any case, (default: %s)", "relay" is wrong, no?
done.
💬 stratospher commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418333665)
done.
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418333665)
done.
💬 stratospher commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418333746)
done.
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418333746)
done.
💬 stratospher commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418333843)
done.
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418333843)
done.
💬 stratospher commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#issuecomment-1844372874)
thanks @maflcko, @0xb10c, @brunoerg. I've updated the PR to address your comments. Open to more thoughts/suggestions in https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417082950.
(https://github.com/bitcoin/bitcoin/pull/29007#issuecomment-1844372874)
thanks @maflcko, @0xb10c, @brunoerg. I've updated the PR to address your comments. Open to more thoughts/suggestions in https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417082950.
📝 pablomartin4btc opened a pull request: "Fix: Disable both "Mask Values" and "Transaction View" if no wallet is selected"
(https://github.com/bitcoin-core/gui/pull/780)
This PR addresses an issue where, with no wallet selected, enabling "Settings -> Mask values" incorrectly enables the transaction tab when the corresponding checkbox is unticked. Additionally, it ensures that the "Mask values" feature is disabled when no wallet is selected, aligning with the behavior of other actions tied to wallet selection/close events (e.g., "Encrypt Wallet").
<details>
<summary>Current behavior display on master</summary>
This PR addresses an issue where, with no wallet selected, enabling "Settings -> Mask values" incorrectly enables the transaction tab when the corresponding checkbox is unticked. Additionally, it ensures that the "Mask values" feature is disabled when no wallet is selected, aligning with the behavior of other actions tied to wallet selection/close events (e.g., "Encrypt Wallet").
<details>
<summary>Current behavior display on master</summary>
![Peek 2023-12-06 19-18](https://github.com/bi
...
💬 maflcko commented on issue "Build error on Ubuntu 22.04.3 LTS":
(https://github.com/bitcoin/bitcoin/issues/29017#issuecomment-1844814552)
You'll have to run `make clean` before compiling a different version of Bitcoin Core with `make`
(https://github.com/bitcoin/bitcoin/issues/29017#issuecomment-1844814552)
You'll have to run `make clean` before compiling a different version of Bitcoin Core with `make`
💬 maflcko commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1418504777)
Ok, so the reason to put Hex into crypto is because it happens to be convenient at this time? No objection, but it would be good to extend the motivation a bit. I asked if this is needed, and what would happen if the commit was dropped, so it may be good to include a compile/link failure in the motivation, if there is one.
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1418504777)
Ok, so the reason to put Hex into crypto is because it happens to be convenient at this time? No objection, but it would be good to extend the motivation a bit. I asked if this is needed, and what would happen if the commit was dropped, so it may be good to include a compile/link failure in the motivation, if there is one.
💬 maflcko commented on pull request "wip: Split fuzz binary":
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1844829261)
> This seems like it's reverting #20560?
yeah, in the current form this is reverting that pull, so I don't think it can be merged. Though, I guess the goal is to somehow come up with magic makefile code (which can optionally be enabled) to extend the build code from the file names automatically.
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1844829261)
> This seems like it's reverting #20560?
yeah, in the current form this is reverting that pull, so I don't think it can be merged. Though, I guess the goal is to somehow come up with magic makefile code (which can optionally be enabled) to extend the build code from the file names automatically.