💬 maflcko commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1812855024)
Yeah, this is a bit confusing. `UndefinedBehaviorSanitizer` is just the name of the tool, not an implication that the issue it found is UB.
If you want to reproduce, you'll have to compile with the `integer` sanitizer (yes, to add to the confusion, the tool is called different here). Also, it only works with `clang++`/`clang`, so you'll have to select that as well during `cmake -B`. Maybe this can be documented in `doc/developer-notes.md`?
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1812855024)
Yeah, this is a bit confusing. `UndefinedBehaviorSanitizer` is just the name of the tool, not an implication that the issue it found is UB.
If you want to reproduce, you'll have to compile with the `integer` sanitizer (yes, to add to the confusion, the tool is called different here). Also, it only works with `clang++`/`clang`, so you'll have to select that as well during `cmake -B`. Maybe this can be documented in `doc/developer-notes.md`?
💬 sipa commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812858116)
I haven't yet experimented with them, but as far as I understand it, coroutines are just programming paradigm, not magic; they don't do anything of their own, besides making things that were already possible easier to write. In particular, you still need a thread pool or some mechanism for scheduling how to run them,
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812858116)
I haven't yet experimented with them, but as far as I understand it, coroutines are just programming paradigm, not magic; they don't do anything of their own, besides making things that were already possible easier to write. In particular, you still need a thread pool or some mechanism for scheduling how to run them,
💬 laanwj commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1812862264)
Might want to keep this arg around for one major version, but make it display an error that the functionality has been removed. This gives a clearer error to anyone who has it in their configuration file than "argument doesn't exist".
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1812862264)
Might want to keep this arg around for one major version, but make it display an error that the functionality has been removed. This gives a clearer error to anyone who has it in their configuration file than "argument doesn't exist".
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812862679)
> We could collect the values to different vectors, each one accessed only by a single thread and merge them into the cache at the end on a single thread
If the vectors are thread local, then how can the main thread access them at the end to write them? We also want to be writing throughout while the workers are fetching, not just at the end.
> How would mpsc solve this better?
Instead of each worker thread having a local queue of results, which they then append to the global results qu
...
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812862679)
> We could collect the values to different vectors, each one accessed only by a single thread and merge them into the cache at the end on a single thread
If the vectors are thread local, then how can the main thread access them at the end to write them? We also want to be writing throughout while the workers are fetching, not just at the end.
> How would mpsc solve this better?
Instead of each worker thread having a local queue of results, which they then append to the global results qu
...
💬 laanwj commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2432339381)
> Concept ACK but shouldn't this go through a deprecation cycle?
imo a real deprecation cycle is not needed here. This has teetered on the brink of being removed for so long already. Also, deprecation cycles are generally for APIs, and this doesn't add or remove any RPC or REST functionality.
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2432339381)
> Concept ACK but shouldn't this go through a deprecation cycle?
imo a real deprecation cycle is not needed here. This has teetered on the brink of being removed for so long already. Also, deprecation cycles are generally for APIs, and this doesn't add or remove any RPC or REST functionality.
🤔 instagibbs requested changes to a pull request: "rpc: getorphantxs follow-up"
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2388886132)
I really think `ORPHAN_MAX_RETENTION_TIME` should be called`ORPHAN_TX_EXPIRE_TIME`
after that LGTM
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2388886132)
I really think `ORPHAN_MAX_RETENTION_TIME` should be called`ORPHAN_TX_EXPIRE_TIME`
after that LGTM
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812869347)
If you a benchmark shows that it is better, than great!
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812869347)
If you a benchmark shows that it is better, than great!
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812874933)
The main thread adds all outpoints to a global vector, which all workers will fetch there work from.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812874933)
The main thread adds all outpoints to a global vector, which all workers will fetch there work from.
💬 instagibbs commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1812876565)
two empty string might be simpler?
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1812876565)
two empty string might be simpler?
🤔 furszy reviewed a pull request: "validation: fetch block inputs on parallel threads ~17% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-2388943653)
Cool idea.
Slightly orthogonal topic:
Since the inputs fetcher call is blocking, instead of creating a new set of worker threads, what do you think about re-using the existing script validation ones (or any other unused worker threads) by implementing a general-purpose thread pool shared among the validation checks?
The script validation checks and the inputs fetching mechanism are never done concurrently because you need the inputs in order to verify the scripts. So, they could share work
...
(https://github.com/bitcoin/bitcoin/pull/31132#pullrequestreview-2388943653)
Cool idea.
Slightly orthogonal topic:
Since the inputs fetcher call is blocking, instead of creating a new set of worker threads, what do you think about re-using the existing script validation ones (or any other unused worker threads) by implementing a general-purpose thread pool shared among the validation checks?
The script validation checks and the inputs fetching mechanism are never done concurrently because you need the inputs in order to verify the scripts. So, they could share work
...
💬 sipa commented on pull request "rpc, cli: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2432418782)
So overall, this is effectively changing from estimating progress based on time, to estimating progress based on comparing with the best headers chain. I think that's a reasonable thing to do, but I would suggest doing it more completely and cleanly, and add some safeguards.
* Guess an "end of chain timestamp" as the maximum of:
* The timestamp of the active chain tip
* The best headers tip's timestamp if it has more work than minchainwork; the current timestamp otherwise.
* Use that t
...
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2432418782)
So overall, this is effectively changing from estimating progress based on time, to estimating progress based on comparing with the best headers chain. I think that's a reasonable thing to do, but I would suggest doing it more completely and cleanly, and add some safeguards.
* Guess an "end of chain timestamp" as the maximum of:
* The timestamp of the active chain tip
* The best headers tip's timestamp if it has more work than minchainwork; the current timestamp otherwise.
* Use that t
...
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-2432481201)
> implementing a general-purpose thread pool shared among the validation checks?
Nice, yes that would be great! That would simplify this PR a lot if it could just schedule tasks on worker threads and receive the responses, instead of implementing all the sync code itself.
> https://github.com/bitcoin/bitcoin/pull/26966 introduces such structure inside https://github.com/bitcoin/bitcoin/commit/401f21bfd72f32a28147677af542887518a4dbff, which we could pull off and use for validation.
Conce
...
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-2432481201)
> implementing a general-purpose thread pool shared among the validation checks?
Nice, yes that would be great! That would simplify this PR a lot if it could just schedule tasks on worker threads and receive the responses, instead of implementing all the sync code itself.
> https://github.com/bitcoin/bitcoin/pull/26966 introduces such structure inside https://github.com/bitcoin/bitcoin/commit/401f21bfd72f32a28147677af542887518a4dbff, which we could pull off and use for validation.
Conce
...
💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r1303702691)
We use `uint32_t` for `block_height` in the previous commit. Any reason we use `int` here?
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r1303702691)
We use `uint32_t` for `block_height` in the previous commit. Any reason we use `int` here?
💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r1812965559)
If we receive an interrupt, we don't also want to wait for the work queue to be empty right?
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r1812965559)
If we receive an interrupt, we don't also want to wait for the work queue to be empty right?
💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r1812986201)
Can we modify this to be more generic and return a type? And add logic to collect all returned values into a shared vector which can then be atomically swapped out by an observer? Possibly not in this PR, but if this will be split out into a generic thread pool.
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r1812986201)
Can we modify this to be more generic and return a type? And add logic to collect all returned values into a shared vector which can then be atomically swapped out by an observer? Possibly not in this PR, but if this will be split out into a generic thread pool.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1812987095)
working on a test and fix for this.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1812987095)
working on a test and fix for this.
💬 maflcko commented on pull request "rpc: getorphantxs follow-up":
(https://github.com/bitcoin/bitcoin/pull/31043#issuecomment-2432543952)
Would be good to address https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788273274 as well. Otherwise there will be another follow-up?
(https://github.com/bitcoin/bitcoin/pull/31043#issuecomment-2432543952)
Would be good to address https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788273274 as well. Otherwise there will be another follow-up?
⚠️ ismaelsadeeq opened an issue: "RPC: `getblockstats` might not return the *effective* percentile fee rate"
(https://github.com/bitcoin/bitcoin/issues/31140)
One of the responses from the `getblockstats` RPC is `feerate_percentiles`, which returns the following:
```zsh
"feerate_percentiles": [ (json array, optional) Feerates at the 10th, 25th, 50th, 75th, and 90th percentile weight unit (in satoshis per virtual byte)
n, (numeric) The 10th percentile feerate
n, (numeric) The 25th percentile feerate
n, (numeric) The 50th percentile feerate
n,
...
(https://github.com/bitcoin/bitcoin/issues/31140)
One of the responses from the `getblockstats` RPC is `feerate_percentiles`, which returns the following:
```zsh
"feerate_percentiles": [ (json array, optional) Feerates at the 10th, 25th, 50th, 75th, and 90th percentile weight unit (in satoshis per virtual byte)
n, (numeric) The 10th percentile feerate
n, (numeric) The 25th percentile feerate
n, (numeric) The 50th percentile feerate
n,
...
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1813017536)
> coroutines are just programming paradigm, not magic
That's also what I was counting on! :D
In RocksDB they have [high and low priority work](https://github.com/facebook/rocksdb/wiki/RocksDB-Tuning-Guide#parallelism-options) (I assume that's just added to the front or the back of a background work deque) – this would align well with @furszy's suggestion for mixing different kinds of background work units.
I haven't used the C++ variant of coroutines either, but my thinking was that sin
...
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1813017536)
> coroutines are just programming paradigm, not magic
That's also what I was counting on! :D
In RocksDB they have [high and low priority work](https://github.com/facebook/rocksdb/wiki/RocksDB-Tuning-Guide#parallelism-options) (I assume that's just added to the front or the back of a background work deque) – this would align well with @furszy's suggestion for mixing different kinds of background work units.
I haven't used the C++ variant of coroutines either, but my thinking was that sin
...
💬 maflcko commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#discussion_r1813038485)
Are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/31093#discussion_r1813038485)
Are you still working on this?