💬 maflcko commented on pull request "fuzz: Limit p2p fuzz targets to MAX_PROTOCOL_MESSAGE_LENGTH":
(https://github.com/bitcoin/bitcoin/pull/29079#issuecomment-1855953721)
> I didn't think oss-fuzz would produce inputs this large
Same. I guess it was found with `use_value_profile=1` storing inputs hitting the `MAX_PROTOCOL_MESSAGE_LENGTH` comparison. :man_shrugging:
(https://github.com/bitcoin/bitcoin/pull/29079#issuecomment-1855953721)
> I didn't think oss-fuzz would produce inputs this large
Same. I guess it was found with `use_value_profile=1` storing inputs hitting the `MAX_PROTOCOL_MESSAGE_LENGTH` comparison. :man_shrugging:
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1425836162)
can't we toss `CheckV3Inheritance` entirely and just use `std::all_of` check, since we've already checked that there exists one v3 already?
e.g.,
```
const bool all_v3{std::all_of(package.cbegin(), package.cend(), [](const auto& tx){ return tx->nVersion == 3; })};
// Check inheritance rules within package.
if (!all_v3) {
// We already checked there was one at least
Assume(std::any_of(package.cbegin(), package.cend(), [](const auto& tx){ return tx->nVersion
...
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1425836162)
can't we toss `CheckV3Inheritance` entirely and just use `std::all_of` check, since we've already checked that there exists one v3 already?
e.g.,
```
const bool all_v3{std::all_of(package.cbegin(), package.cend(), [](const auto& tx){ return tx->nVersion == 3; })};
// Check inheritance rules within package.
if (!all_v3) {
// We already checked there was one at least
Assume(std::any_of(package.cbegin(), package.cend(), [](const auto& tx){ return tx->nVersion
...
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1425837509)
```Suggestion
if (package.size() > V3_ANCESTOR_LIMIT) {
```
since we already know the whole package is v3
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1425837509)
```Suggestion
if (package.size() > V3_ANCESTOR_LIMIT) {
```
since we already know the whole package is v3
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426795358)
If parent is v3 but child v2, we wouldn't check, but you added the extra `else if` which catches that. lgtm
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426795358)
If parent is v3 but child v2, we wouldn't check, but you added the extra `else if` which catches that. lgtm
💬 brunoerg commented on pull request "fuzz: coinselection, improve `min_viable_change`/`change_output_size`":
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1855957951)
Also, I think it would be more appropriate if we don't set `change_spend_size` randomly. Perhaps using `CalculateMaximumSignedInputSize` and the same approach in `CreateTransactionInternal`.
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1855957951)
Also, I think it would be more appropriate if we don't set `change_spend_size` randomly. Perhaps using `CalculateMaximumSignedInputSize` and the same approach in `CreateTransactionInternal`.
💬 murchandamus commented on pull request "fuzz: coinselection, improve `min_viable_change`/`change_output_size`":
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1855990090)
I think it would be fine to roll input and output sizes randomly as long as they remain greater than zero and all the related values are derived from them. E.g. it would be an issue if `cost_of_change` or `min_viable_change` were independent of `change_spend_size`
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1855990090)
I think it would be fine to roll input and output sizes randomly as long as they remain greater than zero and all the related values are derived from them. E.g. it would be an issue if `cost_of_change` or `min_viable_change` were independent of `change_spend_size`
💬 fanquake commented on pull request "ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid unrelated failures":
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1855993456)
> For example,.
> Image Release: https://github.com/actions/runner-images/releases/tag/macos-13%2F20231025.2
> vs
> Image Release: https://github.com/actions/runner-images/releases/tag/macos-13%2F20231204.4
I still don't really understand this fix, and assume it could lead to obscure breakage in the future, i.e a missing dep, or a dep trying to use an outdated sub dep. (it also just makes our CI further differ from nomal macOS / dev setups).
I don't really understand how a new image bei
...
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1855993456)
> For example,.
> Image Release: https://github.com/actions/runner-images/releases/tag/macos-13%2F20231025.2
> vs
> Image Release: https://github.com/actions/runner-images/releases/tag/macos-13%2F20231204.4
I still don't really understand this fix, and assume it could lead to obscure breakage in the future, i.e a missing dep, or a dep trying to use an outdated sub dep. (it also just makes our CI further differ from nomal macOS / dev setups).
I don't really understand how a new image bei
...
💬 brunoerg commented on pull request "fuzz: coinselection, improve `min_viable_change`/`change_output_size`":
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1855999295)
> I think it would be fine to roll input and output sizes randomly as long as they remain greater than zero and all the related values are derived from them. E.g. it would be an issue if cost_of_change or min_viable_change were independent of change_spend_size
But doesn't `change_spend_size` depend on `change_prototype_txout`?
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1855999295)
> I think it would be fine to roll input and output sizes randomly as long as they remain greater than zero and all the related values are derived from them. E.g. it would be an issue if cost_of_change or min_viable_change were independent of change_spend_size
But doesn't `change_spend_size` depend on `change_prototype_txout`?
👍 hernanmarino approved a pull request: "Make bitcoin-tx replaceable value optional"
(https://github.com/bitcoin/bitcoin/pull/29022#pullrequestreview-1782007036)
Tested ACK 98afe7866185ed4157ffc581763e11dc02fcbae0
(https://github.com/bitcoin/bitcoin/pull/29022#pullrequestreview-1782007036)
Tested ACK 98afe7866185ed4157ffc581763e11dc02fcbae0
💬 ryanofsky commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426824485)
In commit "ArgsManager: return path by value from GetBlocksDirPath()" (856c88776f8486446602476a1c9e133ac0cff510)
Might be good to add documention for this method. Maybe "Return a UTF-8 representation of the path as a std::string, for compatibility with code using std::string. For code using the newer std::u8string type, it is more efficient to call the inherited std::filesystem::path::u8string method instead." to explain why this exists and also point out that there is an alternative.
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426824485)
In commit "ArgsManager: return path by value from GetBlocksDirPath()" (856c88776f8486446602476a1c9e133ac0cff510)
Might be good to add documention for this method. Maybe "Return a UTF-8 representation of the path as a std::string, for compatibility with code using std::string. For code using the newer std::u8string type, it is more efficient to call the inherited std::filesystem::path::u8string method instead." to explain why this exists and also point out that there is an alternative.
👍 ryanofsky approved a pull request: "refactor: Remove pre-C++20 code, fs::path cleanup"
(https://github.com/bitcoin/bitcoin/pull/29040#pullrequestreview-1781919113)
Code review ACK facdafaea8cfc2a37e55173d0500794c80b28f7f.
Only change since last review is a new commit renaming the fs::path::u8string method.
(https://github.com/bitcoin/bitcoin/pull/29040#pullrequestreview-1781919113)
Code review ACK facdafaea8cfc2a37e55173d0500794c80b28f7f.
Only change since last review is a new commit renaming the fs::path::u8string method.
💬 ryanofsky commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426781557)
re: https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426458856
> > Maybe declaring the fs::path& path const, and writing to m_cached_blocks_path directly could make the warning go away?
>
> Would that be like tricking the compiler, going around the warning without actually removing the underlying potential problem?
I'm actually not sure the change I'm suggesting would be enough to silence the compiler warning, so it may be a moot point. But the compiler just has incomplete inf
...
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426781557)
re: https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426458856
> > Maybe declaring the fs::path& path const, and writing to m_cached_blocks_path directly could make the warning go away?
>
> Would that be like tricking the compiler, going around the warning without actually removing the underlying potential problem?
I'm actually not sure the change I'm suggesting would be enough to silence the compiler warning, so it may be a moot point. But the compiler just has incomplete inf
...
💬 ryanofsky commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426789849)
re: https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422242907
> Not sure if it makes sense to update the whole codebase from `std::string` to `std::u8string`, just so that UniValue can accept an `std::u8string`. I think it would be fine for UniValue to accept both string types, though, then one would have to be "converted"/copied to the other (I don't think reinterpret_cast+std::move is guaranteed to work from `char` strings to `char8_t` strings, is it?)
Yeah I don't think it ma
...
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426789849)
re: https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422242907
> Not sure if it makes sense to update the whole codebase from `std::string` to `std::u8string`, just so that UniValue can accept an `std::u8string`. I think it would be fine for UniValue to accept both string types, though, then one would have to be "converted"/copied to the other (I don't think reinterpret_cast+std::move is guaranteed to work from `char` strings to `char8_t` strings, is it?)
Yeah I don't think it ma
...
💬 sipa commented on pull request "fuzz: Improve fuzzing stability for minisketch harness":
(https://github.com/bitcoin/bitcoin/pull/29064#issuecomment-1856018932)
@dergoegge I believe that what's going on is that the new fuzz test invokes Merge on sketching with different implementation values. The library detects this and returns 0 from `minisketch_merge`, but the C++ wrapper `Minisketch::Merge` ignores the library result.
(https://github.com/bitcoin/bitcoin/pull/29064#issuecomment-1856018932)
@dergoegge I believe that what's going on is that the new fuzz test invokes Merge on sketching with different implementation values. The library detects this and returns 0 from `minisketch_merge`, but the C++ wrapper `Minisketch::Merge` ignores the library result.
💬 hebasto commented on pull request "ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid unrelated failures":
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1856028951)
>... so I don't understand how anything could be using this image, or how it's causing failures.
This PR does nothing about new `20231204` release, because the failure was caused by the older `20231025` one.
I think that the GHA image release process issues are irrelevant to this PR change.
> I still don't really understand this fix, and assume it could lead to obscure breakage in the future, i.e a missing dep, or a dep trying to use an outdated sub dep. (it also just makes our CI furth
...
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1856028951)
>... so I don't understand how anything could be using this image, or how it's causing failures.
This PR does nothing about new `20231204` release, because the failure was caused by the older `20231025` one.
I think that the GHA image release process issues are irrelevant to this PR change.
> I still don't really understand this fix, and assume it could lead to obscure breakage in the future, i.e a missing dep, or a dep trying to use an outdated sub dep. (it also just makes our CI furth
...
👍 vasild approved a pull request: "refactor: Remove pre-C++20 code, fs::path cleanup"
(https://github.com/bitcoin/bitcoin/pull/29040#pullrequestreview-1782041414)
ACK facdafaea8cfc2a37e55173d0500794c80b28f7f
To verify that nothing has been forgotten in the last commit which renames `fs::path::u8string()` to `fs::path::utf8string()` I added `std::u8string u8string() const = delete;`. The only call to that function is in `BOOST_CHECK(fs::path(str8).u8string() == str8);` which I guess is intentional.
(https://github.com/bitcoin/bitcoin/pull/29040#pullrequestreview-1782041414)
ACK facdafaea8cfc2a37e55173d0500794c80b28f7f
To verify that nothing has been forgotten in the last commit which renames `fs::path::u8string()` to `fs::path::utf8string()` I added `std::u8string u8string() const = delete;`. The only call to that function is in `BOOST_CHECK(fs::path(str8).u8string() == str8);` which I guess is intentional.
👍 maflcko approved a pull request: "logging: Simplify API for level based logging"
(https://github.com/bitcoin/bitcoin/pull/28318#pullrequestreview-1781956013)
Feel free to ignore the nits.
very nice, ACK 757b4c7776f4d11b9860bc93c514d8fea5ef1db 🎺
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trus
...
(https://github.com/bitcoin/bitcoin/pull/28318#pullrequestreview-1781956013)
Feel free to ignore the nits.
very nice, ACK 757b4c7776f4d11b9860bc93c514d8fea5ef1db 🎺
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trus
...
💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426803120)
nit in 9f71029192282baa4f7be6b5124bb7bc2491ef84: Could mark commit as "refactor", if it is one?
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426803120)
nit in 9f71029192282baa4f7be6b5124bb7bc2491ef84: Could mark commit as "refactor", if it is one?
💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426843022)
nit in db5ed8682f99c64a97e598bda69147307b447820: comma before etc?
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426843022)
nit in db5ed8682f99c64a97e598bda69147307b447820: comma before etc?
💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426847187)
nit in https://github.com/bitcoin/bitcoin/commit/db5ed8682f99c64a97e598bda69147307b447820: Clarify "production system *with enough free storage space*". IIRC ~1TB per 100 connection-years? So if you have 100 connections and run for a year, 1TB of your storage will be eaten by transaction relay `net` debug messages, or so.
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426847187)
nit in https://github.com/bitcoin/bitcoin/commit/db5ed8682f99c64a97e598bda69147307b447820: Clarify "production system *with enough free storage space*". IIRC ~1TB per 100 connection-years? So if you have 100 connections and run for a year, 1TB of your storage will be eaten by transaction relay `net` debug messages, or so.