💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166251339)
Thanks for using a more elaborate name, using `first` variable name here was not resonating with me.
This previous suggestion (https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2161706514) also suggested to inverse the boolean values, otherwise this block now reads "if any key is parsed, then set error", which is not right.
I can fix this in follow-up though.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166251339)
Thanks for using a more elaborate name, using `first` variable name here was not resonating with me.
This previous suggestion (https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2161706514) also suggested to inverse the boolean values, otherwise this block now reads "if any key is parsed, then set error", which is not right.
I can fix this in follow-up though.
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166288334)
Interesting case fuzzer caught - the error message made me curious because it didn't contain the `pk` prefix and that made me debug the flow a bit. This case triggered the Miniscript parsing flow in `ParseScript` function and the error is coming from this line in `KeyParser`.
```cpp
auto pk = ParsePubkey(exp_index, {&*begin, &*end}, ParseContext(), *m_out, m_key_parsing_error);
```
I don't think this requires any change in this PR.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166288334)
Interesting case fuzzer caught - the error message made me curious because it didn't contain the `pk` prefix and that made me debug the flow a bit. This case triggered the Miniscript parsing flow in `ParseScript` function and the error is coming from this line in `KeyParser`.
```cpp
auto pk = ParsePubkey(exp_index, {&*begin, &*end}, ParseContext(), *m_out, m_key_parsing_error);
```
I don't think this requires any change in this PR.
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166272447)
Fine to use `Assume` but we still end up with an uncovered code block because it is unreachable.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166272447)
Fine to use `Assume` but we still end up with an uncovered code block because it is unreachable.
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166303215)
```diff
+ * @param[out] has_hardened updated if hardened derivation is found
```
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166303215)
```diff
+ * @param[out] has_hardened updated if hardened derivation is found
```
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166263326)
`span` variable is used only here, I tried to pass `sp` instead and got this error.
```
note: candidate function not viable: 2nd argument ('const std::span<const char>') would lose const qualifier
19 | bool Const(const std::string& str, std::span<const char>& sp, bool skip = true);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
```
I wish there was a way to inform the compiler that the 2nd argument here is not updated if `skip` is false (which
...
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166263326)
`span` variable is used only here, I tried to pass `sp` instead and got this error.
```
note: candidate function not viable: 2nd argument ('const std::span<const char>') would lose const qualifier
19 | bool Const(const std::string& str, std::span<const char>& sp, bool skip = true);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
```
I wish there was a way to inform the compiler that the 2nd argument here is not updated if `skip` is false (which
...
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166544023)
Review note: Besides the internal use within `GetKeyIDs`, I see this function is actually used in the Musig signing PR.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166544023)
Review note: Besides the internal use within `GetKeyIDs`, I see this function is actually used in the Musig signing PR.
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166469077)
> The internal keys
I assume it's referring to the taproot internal key(s).
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166469077)
> The internal keys
I assume it's referring to the taproot internal key(s).
💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166549418)
I see that I had misunderstood your previous comment, this^ looks fine to me.
> The Sign algorithm must not be executed twice with the same secnonce. Otherwise, it is possible to extract the secret signing key from the two partial signatures output by the two executions of Sign.
This^ is the only precaution I read in BIP 327 related to reusing, and I don't think anything in the above comment goes against this precaution. Specially since using the same partial sig doesn't translate to signi
...
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166549418)
I see that I had misunderstood your previous comment, this^ looks fine to me.
> The Sign algorithm must not be executed twice with the same secnonce. Otherwise, it is possible to extract the secret signing key from the two partial signatures output by the two executions of Sign.
This^ is the only precaution I read in BIP 327 related to reusing, and I don't think anything in the above comment goes against this precaution. Specially since using the same partial sig doesn't translate to signi
...
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3004555912)
@glozow
> Alternatively, we could add a `set` of reconsiderable wtxids
I think this is the best option. An extra index does not seem worth it, as it's 3 pointers extra per announcement, while a set of reconsiderable wtxids is just 3-4 pointers per reconsiderable wtxid. Another alternative is making the `ByWtxid` index be `(Wtxid, bool, NodeId)`, but that means that some queries (those which want to check for a specific Wtxid/NodeId announcement) need two lookups. The separate set seems mo
...
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3004555912)
@glozow
> Alternatively, we could add a `set` of reconsiderable wtxids
I think this is the best option. An extra index does not seem worth it, as it's 3 pointers extra per announcement, while a set of reconsiderable wtxids is just 3-4 pointers per reconsiderable wtxid. Another alternative is making the `ByWtxid` index be `(Wtxid, bool, NodeId)`, but that means that some queries (those which want to check for a specific Wtxid/NodeId announcement) need two lookups. The separate set seems mo
...
💬 fanquake commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#issuecomment-3004606908)
Should get a release note, and likely also going to be backported to `29.x`.
(https://github.com/bitcoin/bitcoin/pull/32530#issuecomment-3004606908)
Should get a release note, and likely also going to be backported to `29.x`.
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166635000)
`100'000` isn't sufficient for my machine with an empty destructor. But `200'000` was for both, decreased.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166635000)
`100'000` isn't sufficient for my machine with an empty destructor. But `200'000` was for both, decreased.
💬 rkrux commented on pull request "wallet: remove dead code in legacy wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32758#issuecomment-3004688762)
Thanks for highlighting.
I'll take a look at #31423 soon, it had fallen off my radar for some reason.
(https://github.com/bitcoin/bitcoin/pull/32758#issuecomment-3004688762)
Thanks for highlighting.
I'll take a look at #31423 soon, it had fallen off my radar for some reason.
💬 fanquake commented on pull request "cmake: Use `HINTS` instead of `PATHS` in `find_*` commands":
(https://github.com/bitcoin/bitcoin/pull/32805#issuecomment-3004717577)
cc @purpleKarrot
(https://github.com/bitcoin/bitcoin/pull/32805#issuecomment-3004717577)
cc @purpleKarrot
💬 fanquake commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2166689603)
> If enough participants have joined the effort (5 or more is recommended)
Just noting that of the last three additions to the repo (https://github.com/asmap/asmap-data/pull/23, https://github.com/asmap/asmap-data/pull/21, https://github.com/asmap/asmap-data/pull/19), it seems like none of them reached 5: I can see 3, 3, and 3 participants.
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2166689603)
> If enough participants have joined the effort (5 or more is recommended)
Just noting that of the last three additions to the repo (https://github.com/asmap/asmap-data/pull/23, https://github.com/asmap/asmap-data/pull/21, https://github.com/asmap/asmap-data/pull/19), it seems like none of them reached 5: I can see 3, 3, and 3 participants.
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166642150)
* Reduced indentation in commit message.
* Added validation of the `ScriptSize()` to the test which is a value that depends on child nodes.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166642150)
* Reduced indentation in commit message.
* Added validation of the `ScriptSize()` to the test which is a value that depends on child nodes.
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166653825)
Regarding naming of accessors: There is some precedent in `Node::IsValid()`. But I also prefer it without prefixes - updated.
Regarding `noexcept`: The compiler should be able to infer `noexcept` by itself for simple accessors, so would rather avoid that noise.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166653825)
Regarding naming of accessors: There is some precedent in `Node::IsValid()`. But I also prefer it without prefixes - updated.
Regarding `noexcept`: The compiler should be able to infer `noexcept` by itself for simple accessors, so would rather avoid that noise.
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166650264)
I prefer leaving up to the client of the interface to choose which type they want to switch to.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166650264)
I prefer leaving up to the client of the interface to choose which type they want to switch to.
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166647841)
Done.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166647841)
Done.
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166695337)
Think it's better as is, documenting the properties of the Node inside the optional.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166695337)
Think it's better as is, documenting the properties of the Node inside the optional.
💬 vasild commented on pull request "depends: Build `qt` package for FreeBSD hosts":
(https://github.com/bitcoin/bitcoin/pull/32731#discussion_r2166737725)
I tested the latest version of this PR (173394d951). It compiles successfully with `gmake` and `cmake --toolchain`. And this is the result:
```
$ ldd ./build/bin/bitcoin-qt
./build/bin/bitcoin-qt:
libfontconfig.so.1 => not found (0)
libfreetype.so.6 => not found (0)
libxkbcommon.so.0 => not found (0)
libxkbcommon-x11.so.0 => not found (0)
libxcb-xkb.so.1 => not found (0)
libxcb.so.1 => not found (0)
libxcb-cursor.so.0 => not found (0)
libxcb-icccm.so.4 => not found (0)
lib
...
(https://github.com/bitcoin/bitcoin/pull/32731#discussion_r2166737725)
I tested the latest version of this PR (173394d951). It compiles successfully with `gmake` and `cmake --toolchain`. And this is the result:
```
$ ldd ./build/bin/bitcoin-qt
./build/bin/bitcoin-qt:
libfontconfig.so.1 => not found (0)
libfreetype.so.6 => not found (0)
libxkbcommon.so.0 => not found (0)
libxkbcommon-x11.so.0 => not found (0)
libxcb-xkb.so.1 => not found (0)
libxcb.so.1 => not found (0)
libxcb-cursor.so.0 => not found (0)
libxcb-icccm.so.4 => not found (0)
lib
...