💬 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.
💬 maflcko commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418515159)
> 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.
It would just be a one-line function to hold the code you already wrote in two places:
```cpp
return any_of(args.GetArgs("test"), test_option);
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418515159)
> 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.
It would just be a one-line function to hold the code you already wrote in two places:
```cpp
return any_of(args.GetArgs("test"), test_option);
💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1418530564)
Looking at #28395, the bug is related to SFFO. If we are going to log something, it would be better to log information related to what options the user selected, such as logging that SFFO was selected (I did a quick grep and didn't see that this was being logged, but might have missed it). In general, I think it's always better to log the parameters that were used to start a process, rather than log the resulting algorithm internals.
If we expect BnB to _always_ produce changeless solutions,
...
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1418530564)
Looking at #28395, the bug is related to SFFO. If we are going to log something, it would be better to log information related to what options the user selected, such as logging that SFFO was selected (I did a quick grep and didn't see that this was being logged, but might have missed it). In general, I think it's always better to log the parameters that were used to start a process, rather than log the resulting algorithm internals.
If we expect BnB to _always_ produce changeless solutions,
...
💬 TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1418544480)
If you drop the commit splitting `strencodings` and moving the hex function to the crypto library you get a bunch of errors when compiling the last commit:
```
/usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x23
util/.libs/libbitcoinkernel_la-strencodings.o: in function `SanitizeString[abi:cxx11](std::basic_string_view<char, std::char_traits<char> >, int)':
strencodings.cpp:(.text+0x0): multiple definition of `SanitizeString[abi:cxx11](std::basic_string_view<char, st
...
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1418544480)
If you drop the commit splitting `strencodings` and moving the hex function to the crypto library you get a bunch of errors when compiling the last commit:
```
/usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x23
util/.libs/libbitcoinkernel_la-strencodings.o: in function `SanitizeString[abi:cxx11](std::basic_string_view<char, std::char_traits<char> >, int)':
strencodings.cpp:(.text+0x0): multiple definition of `SanitizeString[abi:cxx11](std::basic_string_view<char, st
...
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1418548663)
as long as you reack a new version it's fine, since no other acks are pending yet :)
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1418548663)
as long as you reack a new version it's fine, since no other acks are pending yet :)
💬 naumenkogs commented on pull request "fuzz: p2p: Detect peer deadlocks":
(https://github.com/bitcoin/bitcoin/pull/29009#issuecomment-1844877620)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29009#issuecomment-1844877620)
Concept ACK
💬 0xB10C commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418564026)
I've been thinking about how to clear the addrman tables in a functional test. I think it would be ideal if we could avoid the `addpeeraddress` test _leaking_ into `getaddrmaninfo` and `getrawaddrman` tests.
This also becomes relevant in the context of #28998 where we might want to produce a tried table collision on purpose to have coverage of the `addpeeraddress` RPC failure path. The easiest way of producing a collision is by adding more and more addresses to the tried table (as we do in t
...
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418564026)
I've been thinking about how to clear the addrman tables in a functional test. I think it would be ideal if we could avoid the `addpeeraddress` test _leaking_ into `getaddrmaninfo` and `getrawaddrman` tests.
This also becomes relevant in the context of #28998 where we might want to produce a tried table collision on purpose to have coverage of the `addpeeraddress` RPC failure path. The easiest way of producing a collision is by adding more and more addresses to the tried table (as we do in t
...