💬 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.
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1729052874)
@sipa I psyop'd myself into thinking they were connected... somehow.. Well, I believe your example!
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1729052874)
@sipa I psyop'd myself into thinking they were connected... somehow.. Well, I believe your example!
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729049664)
Might do if I re-touch.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729049664)
Might do if I re-touch.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729053334)
Yeah, my goal was to contrast it against `ParseHex` returning a vector. Will do if I re-touch.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729053334)
Yeah, my goal was to contrast it against `ParseHex` returning a vector. Will do if I re-touch.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729064946)
I want to explicitly enforce compile time where possible, hence the `consteval`.
In the `""_hex_v_u8` case, `constexpr` would be a lie, as `UCharCast` being called is `inline`, not `constexpr`, and calls `reinterpret_cast` which is not supported at compile time. (This means that `UCharSpanCast` and `MakeUCharSpan` are `constexpr` in name only, except for if called on a collection that is already `unsigned char` and doesn't require `reinterpret_cast`).
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729064946)
I want to explicitly enforce compile time where possible, hence the `consteval`.
In the `""_hex_v_u8` case, `constexpr` would be a lie, as `UCharCast` being called is `inline`, not `constexpr`, and calls `reinterpret_cast` which is not supported at compile time. (This means that `UCharSpanCast` and `MakeUCharSpan` are `constexpr` in name only, except for if called on a collection that is already `unsigned char` and doesn't require `reinterpret_cast`).
💬 martinsaposnic commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#issuecomment-2307233648)
Out of curiosity I ran a little benchmark
Master:
<img width="607" alt="image" src="https://github.com/user-attachments/assets/875f96fd-5ef2-4730-a3df-f7b3ff40f039">
With MiniWallet:
<img width="903" alt="image" src="https://github.com/user-attachments/assets/40bdcb95-1b08-4054-9223-44566bdca548">
Miniwallet is cool
(https://github.com/bitcoin/bitcoin/pull/30701#issuecomment-2307233648)
Out of curiosity I ran a little benchmark
Master:
<img width="607" alt="image" src="https://github.com/user-attachments/assets/875f96fd-5ef2-4730-a3df-f7b3ff40f039">
With MiniWallet:
<img width="903" alt="image" src="https://github.com/user-attachments/assets/40bdcb95-1b08-4054-9223-44566bdca548">
Miniwallet is cool
💬 martinsaposnic commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#issuecomment-2307235414)
@theStack I implemented the 3 suggestions. Let me know if you have further comments 🙂
(https://github.com/bitcoin/bitcoin/pull/30701#issuecomment-2307235414)
@theStack I implemented the 3 suggestions. Let me know if you have further comments 🙂