💬 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 🙂
💬 virtu commented on issue "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does":
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2307236913)
Only bumping this because there's a good chance this seed will serve zero addresses by release time.
Seeder been stuck [since January](https://21.ninja/dns-seeds/advertised-count/) and has deteriorated from 37 down to currently 5 addresses:
```bash
» for addr in seed.bitcoinstats.com x9.seed.bitcoinstats.com; do echo -n "unique addresses from $addr: " && host $addr | grep address | awk '{print $NF}' | sort -u | wc -l ; done
unique addresses from seed.bitcoinstats.com: 5
unique addresses
...
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2307236913)
Only bumping this because there's a good chance this seed will serve zero addresses by release time.
Seeder been stuck [since January](https://21.ninja/dns-seeds/advertised-count/) and has deteriorated from 37 down to currently 5 addresses:
```bash
» for addr in seed.bitcoinstats.com x9.seed.bitcoinstats.com; do echo -n "unique addresses from $addr: " && host $addr | grep address | awk '{print $NF}' | sort -u | wc -l ; done
unique addresses from seed.bitcoinstats.com: 5
unique addresses
...
🚀 fanquake merged a pull request: "[27.x] Even more backports"
(https://github.com/bitcoin/bitcoin/pull/30558)
(https://github.com/bitcoin/bitcoin/pull/30558)
💬 furszy commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#discussion_r1729102053)
> In commit "init: fix fatal error on '-wallet' negated option value" ([e56e2f5](https://github.com/bitcoin/bitcoin/commit/e56e2f51803b276945c74a479d30d884256d7ffc))
>
> If desired, you could include the value in the error message like `strprintf(_("Invalid -wallet value %s detected"), wallet.write())`
Sadly, that wouldn't help much and could end up confusing users. The error output for `-nowallet=not_a_boolean` would be `-wallet=true`, which does not correspond to what the user inputted.
(https://github.com/bitcoin/bitcoin/pull/30684#discussion_r1729102053)
> In commit "init: fix fatal error on '-wallet' negated option value" ([e56e2f5](https://github.com/bitcoin/bitcoin/commit/e56e2f51803b276945c74a479d30d884256d7ffc))
>
> If desired, you could include the value in the error message like `strprintf(_("Invalid -wallet value %s detected"), wallet.write())`
Sadly, that wouldn't help much and could end up confusing users. The error output for `-nowallet=not_a_boolean` would be `-wallet=true`, which does not correspond to what the user inputted.