💬 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>`
💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1418647036)
After your changes, we won't consider `BLOCK_RELAY`, `FEELER`, `ADDR_FETCH` anymore, right?
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1418647036)
After your changes, we won't consider `BLOCK_RELAY`, `FEELER`, `ADDR_FETCH` anymore, right?
🤔 MarnixCroes reviewed a pull request: "Fix: Disable both "Mask Values" and "Transaction View" if no wallet is selected"
(https://github.com/bitcoin-core/gui/pull/780#pullrequestreview-1769626322)
nice find
I'm not sure if this is the right approach: user might want to enable `Mask values` before opening his wallet?
(https://github.com/bitcoin-core/gui/pull/780#pullrequestreview-1769626322)
nice find
I'm not sure if this is the right approach: user might want to enable `Mask values` before opening his wallet?
💬 Sjors commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1844981799)
This does not meet the bar for "good". If only Ocean pool uses this and it remains a relatively small pool, it will have no effect. If it is widely deployed, it's trivial to circumvent, which will cause this code to grow in complexity. And every time that happens there'll be a lot of pressure on maintainers to push it through quickly "to stop the spam".
For example you're currently looking for this pattern, used for inscriptions:
```
OP_FALSE OP_IF spam OP_ENDIF
```
This pattern fits
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1844981799)
This does not meet the bar for "good". If only Ocean pool uses this and it remains a relatively small pool, it will have no effect. If it is widely deployed, it's trivial to circumvent, which will cause this code to grow in complexity. And every time that happens there'll be a lot of pressure on maintainers to push it through quickly "to stop the spam".
For example you're currently looking for this pattern, used for inscriptions:
```
OP_FALSE OP_IF spam OP_ENDIF
```
This pattern fits
...
📝 TheCharlatan opened a pull request: "mempool: Don't sort in entryAll"
(https://github.com/bitcoin/bitcoin/pull/29019)
Current call sites of entryAll do not require the entries to be sorted, so iterate through mapTx directly to retrieve the entries instead of using SortedDepthAndScore.
This is a follow up to #28391 and was noted here https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1418261403.
(https://github.com/bitcoin/bitcoin/pull/29019)
Current call sites of entryAll do not require the entries to be sorted, so iterate through mapTx directly to retrieve the entries instead of using SortedDepthAndScore.
This is a follow up to #28391 and was noted here https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1418261403.
💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1418654682)
Why don't you implement going out of this warning?
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1418654682)
Why don't you implement going out of this warning?
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1418656096)
This function is supposed to mimic `infoAll`, which was originally used in this patchset to retrieve mempool information instead of directly accessing `mapTx`. Looking at its usage you are right, the way I read this too there were no order promises made prior to this patch. I opened https://github.com/bitcoin/bitcoin/pull/29019 to instead just iterate through `mapTx`.
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1418656096)
This function is supposed to mimic `infoAll`, which was originally used in this patchset to retrieve mempool information instead of directly accessing `mapTx`. Looking at its usage you are right, the way I read this too there were no order promises made prior to this patch. I opened https://github.com/bitcoin/bitcoin/pull/29019 to instead just iterate through `mapTx`.