💬 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
...
💬 0xB10C commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418567733)
We're doing 1. in the [feature_addrman.py](https://github.com/bitcoin/bitcoin/blob/2e8ec6b338a825a7155fff1be83993e3834ab655/test/functional/feature_addrman.py#L160-L161) functional test.
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418567733)
We're doing 1. in the [feature_addrman.py](https://github.com/bitcoin/bitcoin/blob/2e8ec6b338a825a7155fff1be83993e3834ab655/test/functional/feature_addrman.py#L160-L161) functional test.
💬 maflcko commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1844894888)
> or in the combination of GCC 11 & LLVM 17
Tried testing this on Jammy, but it seemed fine, or maybe I did something wrong?
```
# riscv64-linux-gnu-g++ --version
riscv64-linux-gnu-g++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
# ninja -C build dsymutil
ninja: Entering directory `buil
...
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1844894888)
> or in the combination of GCC 11 & LLVM 17
Tried testing this on Jammy, but it seemed fine, or maybe I did something wrong?
```
# riscv64-linux-gnu-g++ --version
riscv64-linux-gnu-g++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
# ninja -C build dsymutil
ninja: Entering directory `buil
...
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1844899295)
Repeated my manual tests, all works.
The only thing I want to clarify is the downgrade + encrypt situation. So now the wallet loads and not "corrupted" but if the user would generate new descriptors (as proposed in future PRs) the wallet will use the pre-encryption HD key. The scenario seems pretty rare, but still slightly concerning as the pre-encryption HD key could've been compromised.
Maybe we should consider only loading key from active descriptors? In this case the wallet will "loose
...
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1844899295)
Repeated my manual tests, all works.
The only thing I want to clarify is the downgrade + encrypt situation. So now the wallet loads and not "corrupted" but if the user would generate new descriptors (as proposed in future PRs) the wallet will use the pre-encryption HD key. The scenario seems pretty rare, but still slightly concerning as the pre-encryption HD key could've been compromised.
Maybe we should consider only loading key from active descriptors? In this case the wallet will "loose
...
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1844906757)
Took a minor suggestion from @sr-gi, fixed clang-formatting, fixed code distribution between the two last commits.
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1844906757)
Took a minor suggestion from @sr-gi, fixed clang-formatting, fixed code distribution between the two last commits.
⚠️ maflcko opened an issue: "fuzz: Fix stability, determinism issues"
(https://github.com/bitcoin/bitcoin/issues/29018)
It would be good to track fuzz "stability" and determinism, and fix any issues.
Is there an easy way to generate a table for this metric for each fuzz target, maybe as a side effect of CI, or in another way?
cc @dergoegge
(https://github.com/bitcoin/bitcoin/issues/29018)
It would be good to track fuzz "stability" and determinism, and fix any issues.
Is there an easy way to generate a table for this metric for each fuzz target, maybe as a side effect of CI, or in another way?
cc @dergoegge
💬 maflcko commented on pull request "wip: Split fuzz binary":
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1844917023)
I guess the "one fuzz target per file" makes the "one fuzz binary per message/rpc type" idea a bit harder to implement: https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1621453405 ?
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1844917023)
I guess the "one fuzz target per file" makes the "one fuzz binary per message/rpc type" idea a bit harder to implement: https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1621453405 ?
💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1418616411)
I won't mind, but this should be commented somewhere above — perhaps where 0 returned is handled.
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1418616411)
I won't mind, but this should be commented somewhere above — perhaps where 0 returned is handled.
💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1418629765)
6c65f61b625212e128e5db52cbd80a3b57b1450b
nit: your code assumes that N is less than the capacity of `size_t`, otherwise this addition can overflow. You might consider adding a static assert for that too, just in case someone in the future decides to use `TimeOffsets<bazillion>`
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1418629765)
6c65f61b625212e128e5db52cbd80a3b57b1450b
nit: your code assumes that N is less than the capacity of `size_t`, otherwise this addition can overflow. You might consider adding a static assert for that too, just in case someone in the future decides to use `TimeOffsets<bazillion>`