💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1728903797)
Done, and reorganized the test a bit to use `std::string valid_hex_64` instead of `std::string_view valid_hex_65` which makes cat operations significantly less verbose, and makes the test more readable imo.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1728903797)
Done, and reorganized the test a bit to use `std::string valid_hex_64` instead of `std::string_view valid_hex_65` which makes cat operations significantly less verbose, and makes the test more readable imo.
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1728890062)
Thanks, taken.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1728890062)
Thanks, taken.
💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2307049315)
re-ACK 245dfa20bd37f78783adbd03897b7f144d71a7b8
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2307049315)
re-ACK 245dfa20bd37f78783adbd03897b7f144d71a7b8
💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2307070733)
re-ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156
`git range-diff master 81554aac80bf2270db977c110c37acc7e8034194 18d65d27726bf9fc7629b8e794047a10c9cf6156`
Changes: Grammar, added&adjusted tests (passed), undid unintentional new line.
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2307070733)
re-ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156
`git range-diff master 81554aac80bf2270db977c110c37acc7e8034194 18d65d27726bf9fc7629b8e794047a10c9cf6156`
Changes: Grammar, added&adjusted tests (passed), undid unintentional new line.
💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2307077927)
```
$ FUZZ=txdownload_impl src/test/fuzz/fuzz solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt
test/fuzz/txdownloadman.cpp:384 operator(): Assertion `!txdownload_impl.RecentRejectsFilter().contains(package.back()->GetWitnessHash().ToUint256())' failed.
```
[solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt](https://github.com/user-attachments/files/16729173/solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt)
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2307077927)
```
$ FUZZ=txdownload_impl src/test/fuzz/fuzz solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt
test/fuzz/txdownloadman.cpp:384 operator(): Assertion `!txdownload_impl.RecentRejectsFilter().contains(package.back()->GetWitnessHash().ToUint256())' failed.
```
[solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt](https://github.com/user-attachments/files/16729173/solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt)
💬 hodlinator commented on issue "Unit test failures when using multiple jobs and RANDOM_CTX_SEED":
(https://github.com/bitcoin/bitcoin/issues/30696#issuecomment-2307081520)
Thinking about it some more, it makes sense that if we give all jobs the same random seed and 97e16f57042cab07e5e73f6bed19feec2006e4f7 makes the test folder generation deterministic, the jobs will trample over each other. If that is the case, it's probably better bring back some non-determinism in the folder generation.
(https://github.com/bitcoin/bitcoin/issues/30696#issuecomment-2307081520)
Thinking about it some more, it makes sense that if we give all jobs the same random seed and 97e16f57042cab07e5e73f6bed19feec2006e4f7 makes the test folder generation deterministic, the jobs will trample over each other. If that is the case, it's probably better bring back some non-determinism in the folder generation.
💬 maflcko commented on issue "Unit test failures when using multiple jobs and RANDOM_CTX_SEED":
(https://github.com/bitcoin/bitcoin/issues/30696#issuecomment-2307116476)
Correct. The easiest fix would be to just use `GetStrongRandBytes`, however, I am not sure how slow that'd be in the worst case.
Another possible fix would be to move the SeedRandomForTest after the fuzz target's init function, but before ResetCoverageCounters. However that'd still break if the fuzz target spun up a TestingSetup.
Another possible fix would be to instead use the hash of the current folder generator, as well as the hash of the pid. Which may obviously break once the kernel w
...
(https://github.com/bitcoin/bitcoin/issues/30696#issuecomment-2307116476)
Correct. The easiest fix would be to just use `GetStrongRandBytes`, however, I am not sure how slow that'd be in the worst case.
Another possible fix would be to move the SeedRandomForTest after the fuzz target's init function, but before ResetCoverageCounters. However that'd still break if the fuzz target spun up a TestingSetup.
Another possible fix would be to instead use the hash of the current folder generator, as well as the hash of the pid. Which may obviously break once the kernel w
...
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2307124171)
> [solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt](https://github.com/user-attachments/files/16729173/solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt)
`Find1P1CPackage` doesn't actually check the normal reject filter at all with respect to the child, only reconsiderable, so that assertion gets hit. I suspect that's something we don't want it to do, even if it's not possible to hit inside net_processing.
LGTM modulo existing concerns about the `tx_download_one_honest_peer` harn
...
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2307124171)
> [solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt](https://github.com/user-attachments/files/16729173/solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt)
`Find1P1CPackage` doesn't actually check the normal reject filter at all with respect to the child, only reconsiderable, so that assertion gets hit. I suspect that's something we don't want it to do, even if it's not possible to hit inside net_processing.
LGTM modulo existing concerns about the `tx_download_one_honest_peer` harn
...
🤔 glozow reviewed a pull request: "cluster mempool: optimized candidate search"
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2256916267)
ACK 12760a57b3a
Agree with keeping "reduce computation of unnecessary pot sets", intuitively it seems useful in the average situation and quite cheap.
Thanks for dropping "avoid jump ahead in unmodified pot sets," I wasn't convinced that processing the existing pot txns in the exclusion branch is always redundant or faster to skip. It also made the code harder for future people to understand, which I think is important.
I redid code review, all pretty straightforward now.
Prettified
...
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2256916267)
ACK 12760a57b3a
Agree with keeping "reduce computation of unnecessary pot sets", intuitively it seems useful in the average situation and quite cheap.
Thanks for dropping "avoid jump ahead in unmodified pot sets," I wasn't convinced that processing the existing pot txns in the exclusion branch is always redundant or faster to skip. It also made the code harder for future people to understand, which I think is important.
I redid code review, all pretty straightforward now.
Prettified
...
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1728757747)
781e0fb3aa5: maybe paranoid, but
```suggestion
transactions.Set(pos);
if (Assume(!transactions[pos])) feerate += depgraph.FeeRate(pos);
```
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1728757747)
781e0fb3aa5: maybe paranoid, but
```suggestion
transactions.Set(pos);
if (Assume(!transactions[pos])) feerate += depgraph.FeeRate(pos);
```
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1728910777)
Was trying to come up with some intuition for this bound. With no topological restrictions, we have 2^n subsets (yes/no n times). Minus the empty one. But every tx is in a dependency relationship if the graph is connected, therefore at least half of the subsets are not topological?
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1728910777)
Was trying to come up with some intuition for this bound. With no topological restrictions, we have 2^n subsets (yes/no n times). Minus the empty one. But every tx is in a dependency relationship if the graph is connected, therefore at least half of the subsets are not topological?
💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1728997258)
In commit "util: Add consteval ""_hex[_v][_u8] literals" (f9fed4d4e6daa2bdd715b92b6bfffdbe8202a9c8)
This could be constexpr and I think switching between consteval and constexpr just makes the definitions inconsistent and adds noise. Would suggest:
```diff
--- a/src/util/strencodings.h
+++ b/src/util/strencodings.h
@@ -431,16 +431,16 @@ struct Hex {
} // namespace detail
template <util::detail::Hex str>
-consteval auto operator""_hex() { return str.bytes; }
+constexpr auto oper
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1728997258)
In commit "util: Add consteval ""_hex[_v][_u8] literals" (f9fed4d4e6daa2bdd715b92b6bfffdbe8202a9c8)
This could be constexpr and I think switching between consteval and constexpr just makes the definitions inconsistent and adds noise. Would suggest:
```diff
--- a/src/util/strencodings.h
+++ b/src/util/strencodings.h
@@ -431,16 +431,16 @@ struct Hex {
} // namespace detail
template <util::detail::Hex str>
-consteval auto operator""_hex() { return str.bytes; }
+constexpr auto oper
...
💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1728968413)
In commit "util: Add consteval ""_hex[_v][_u8] literals" (f9fed4d4e6daa2bdd715b92b6bfffdbe8202a9c8)
Maybe change "The reason" to "One reason". I think even if C++ did provide a way to convert `constexpr` vectors to runtime vectors(*) it would still make sense for this to use std::array because:
- Semantically this returns a fixed size array, not a variable sized one
- std::array is more flexible because array values can be allocated on the heap, or on stack, or in static storage, while st
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1728968413)
In commit "util: Add consteval ""_hex[_v][_u8] literals" (f9fed4d4e6daa2bdd715b92b6bfffdbe8202a9c8)
Maybe change "The reason" to "One reason". I think even if C++ did provide a way to convert `constexpr` vectors to runtime vectors(*) it would still make sense for this to use std::array because:
- Semantically this returns a fixed size array, not a variable sized one
- std::array is more flexible because array values can be allocated on the heap, or on stack, or in static storage, while st
...
💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1728947485)
In commit "util: Add consteval ""_hex[_v][_u8] literals" (f9fed4d4e6daa2bdd715b92b6bfffdbe8202a9c8)
> It may be necessary to use _v variants when serializing, or vice versa, because
Would make more sense as "It could be necessary to use vector instead of array variants when serializing, or vice versa, because"
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1728947485)
In commit "util: Add consteval ""_hex[_v][_u8] literals" (f9fed4d4e6daa2bdd715b92b6bfffdbe8202a9c8)
> It may be necessary to use _v variants when serializing, or vice versa, because
Would make more sense as "It could be necessary to use vector instead of array variants when serializing, or vice versa, because"
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1729021009)
I believe the worst case for number of connected topologically valid sets in a connected cluster is a cluster with one parent, and then N-1 independent children.
In this setting, every non-enpty topologically valid subset consists of the parent plus any of the 2^(N-1) subsets of children.
This isn't a proof that nothing worse exists, but I haven't been able to come with anything else.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1729021009)
I believe the worst case for number of connected topologically valid sets in a connected cluster is a cluster with one parent, and then N-1 independent children.
In this setting, every non-enpty topologically valid subset consists of the parent plus any of the 2^(N-1) subsets of children.
This isn't a proof that nothing worse exists, but I haven't been able to come with anything else.
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1729025288)
(in case my reasoning was faulty) My example was N-1 parents and 1 child.
2^(N-1) - 1 possible choices of parents without child
Adding child is one more possibility with all parents selected, resulting in 2^(N-1)?
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1729025288)
(in case my reasoning was faulty) My example was N-1 parents and 1 child.
2^(N-1) - 1 possible choices of parents without child
Adding child is one more possibility with all parents selected, resulting in 2^(N-1)?
👍 ryanofsky approved a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2257343762)
Code review ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156. Very nice change that cleans up the API, adds checking for invalid values, makes parsing of values more consistent, and adds test coverage.
Just small test, error message, and comment changes since last review
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2257343762)
Code review ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156. Very nice change that cleans up the API, adds checking for invalid values, makes parsing of values more consistent, and adds test coverage.
Just small test, error message, and comment changes since last review
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1729040254)
@instagibbs Right, that works too. I was focused on just counting *connected* topologically valid subsets, but that isn't actually required in this context. Good to see it's still the same 2^(N-1) number.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1729040254)
@instagibbs Right, that works too. I was focused on just counting *connected* topologically valid subsets, but that isn't actually required in this context. Good to see it's still the same 2^(N-1) number.
💬 hodlinator commented on issue "Unit test failures when using multiple jobs and RANDOM_CTX_SEED":
(https://github.com/bitcoin/bitcoin/issues/30696#issuecomment-2307174387)
Top of **setup_common.cpp** is a bit damning:
```C++
/** Random context to get unique temp data dirs. Separate from g_insecure_rand_ctx, which can be seeded from a const env var */
static FastRandomContext g_insecure_rand_ctx_temp_path;
```
Together with the comment added in #29625 in **test/util/random.cpp** ("GetRandHash is no longer truly random"):
```C++
// Do this once, on the first call, regardless of seedtype, because once
// MakeRandDeterministicDANGEROUS is called, the o
...
(https://github.com/bitcoin/bitcoin/issues/30696#issuecomment-2307174387)
Top of **setup_common.cpp** is a bit damning:
```C++
/** Random context to get unique temp data dirs. Separate from g_insecure_rand_ctx, which can be seeded from a const env var */
static FastRandomContext g_insecure_rand_ctx_temp_path;
```
Together with the comment added in #29625 in **test/util/random.cpp** ("GetRandHash is no longer truly random"):
```C++
// Do this once, on the first call, regardless of seedtype, because once
// MakeRandDeterministicDANGEROUS is called, the o
...
💬 sipa commented on issue "Unit test failures when using multiple jobs and RANDOM_CTX_SEED":
(https://github.com/bitcoin/bitcoin/issues/30696#issuecomment-2307179633)
@hodlinator I probably won't have time to look into this the next two weeks, but I'm happy to defer to @maflcko on this.
(https://github.com/bitcoin/bitcoin/issues/30696#issuecomment-2307179633)
@hodlinator I probably won't have time to look into this the next two weeks, but I'm happy to defer to @maflcko on this.