💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2500300779)
> two threads working while main is waiting vs one thread working and main also working?
It would be two threads working while main is waiting vs two threads working and main also working? So the latter has 3 threads working vs 2 threads working. If using `-par=3` this is the case. 50% more work is done in parallel.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2500300779)
> two threads working while main is waiting vs one thread working and main also working?
It would be two threads working while main is waiting vs two threads working and main also working? So the latter has 3 threads working vs 2 threads working. If using `-par=3` this is the case. 50% more work is done in parallel.
💬 w0xlt commented on pull request "[kernel] Expose `CheckTransaction` consensus validation function":
(https://github.com/bitcoin/bitcoin/pull/33796#issuecomment-3498877580)
> I am not sure how useful this check by itself is … what is the purpose served from surfacing the context‑free checks, but not the others?
The new API intentionally exposes only the context‑free consensus predicate (`consensus/tx_check::CheckTransaction`) so callers can **fail fast** on malformed transactions without needing a kernel context, UTXO set, or policy knobs.
This gives library users (indexers, gateways, alternative mempool layers, etc.) a **cheap pre‑filter** to catch structura
...
(https://github.com/bitcoin/bitcoin/pull/33796#issuecomment-3498877580)
> I am not sure how useful this check by itself is … what is the purpose served from surfacing the context‑free checks, but not the others?
The new API intentionally exposes only the context‑free consensus predicate (`consensus/tx_check::CheckTransaction`) so callers can **fail fast** on malformed transactions without needing a kernel context, UTXO set, or policy knobs.
This gives library users (indexers, gateways, alternative mempool layers, etc.) a **cheap pre‑filter** to catch structura
...
💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2500379088)
(resolved this conversation, and most of the others, but I am happy to review follow-ups, if there are any)
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2500379088)
(resolved this conversation, and most of the others, but I am happy to review follow-ups, if there are any)
🤔 maflcko requested changes to a pull request: "test: skip interface_ipc if python version is freethreaded and PYTHON_GIL=0 is not set"
(https://github.com/bitcoin/bitcoin/pull/33795#pullrequestreview-3429988977)
not sure about this
(https://github.com/bitcoin/bitcoin/pull/33795#pullrequestreview-3429988977)
not sure about this
💬 maflcko commented on pull request "test: skip interface_ipc if python version is freethreaded and PYTHON_GIL=0 is not set":
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2500400726)
not sure. This seems to go in the wrong direction. It disables a legit? warning without any reason why disabling it would be safe or is even desirable. See https://github.com/bitcoin/bitcoin/issues/33582#issuecomment-3386064377 and https://github.com/bitcoin/bitcoin/pull/33795#issuecomment-3495381887
The correct fix would be to just require the GIL
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2500400726)
not sure. This seems to go in the wrong direction. It disables a legit? warning without any reason why disabling it would be safe or is even desirable. See https://github.com/bitcoin/bitcoin/issues/33582#issuecomment-3386064377 and https://github.com/bitcoin/bitcoin/pull/33795#issuecomment-3495381887
The correct fix would be to just require the GIL
🤔 mzumsande reviewed a pull request: "net: option to disallow v1 connection on ipv4 and ipv6 peers"
(https://github.com/bitcoin/bitcoin/pull/30951#pullrequestreview-3429992454)
> deny v1 incoming (only allow v2 incoming) which is a problem if this is widely adopted because then old nodes that do not support v2 will have a hard time finding peers to connect to and will be subject to eclipse from an attacker who runs many v1-accepting nodes.
I still don't find the reason for tying it to `-listen=0` instead of also disallowing v1 inbounds compelling. Yes, if everyone disabled v1 incoming peers that could lead to v1 nodes being eclipsed. But with the suggested approach,
...
(https://github.com/bitcoin/bitcoin/pull/30951#pullrequestreview-3429992454)
> deny v1 incoming (only allow v2 incoming) which is a problem if this is widely adopted because then old nodes that do not support v2 will have a hard time finding peers to connect to and will be subject to eclipse from an attacker who runs many v1-accepting nodes.
I still don't find the reason for tying it to `-listen=0` instead of also disallowing v1 inbounds compelling. Yes, if everyone disabled v1 incoming peers that could lead to v1 nodes being eclipsed. But with the suggested approach,
...
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2500406322)
So why not just do the work that `par` defined and leave main asleep, wouldn't that be simpler while basically achieving exactly the same?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2500406322)
So why not just do the work that `par` defined and leave main asleep, wouldn't that be simpler while basically achieving exactly the same?
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2500437404)
No because we would have to insert all entries into temp cache at the end, instead of parallelizing that work as well. That was the previous implementation where we waited for every thread to be done and then inserted everything in series before exiting.
Now, the main thread does both. It inserts while others are fetching, but if it inserts fast enough where it is waiting for new entries it will also fetch entries.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2500437404)
No because we would have to insert all entries into temp cache at the end, instead of parallelizing that work as well. That was the previous implementation where we waited for every thread to be done and then inserted everything in series before exiting.
Now, the main thread does both. It inserts while others are fetching, but if it inserts fast enough where it is waiting for new entries it will also fetch entries.
💬 ryanofsky commented on pull request "test: skip interface_ipc if python version is freethreaded and PYTHON_GIL=0 is not set":
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2500447299)
> The correct fix would be to just require the GIL
That's right, the text of the warning (from #33582) states "The global interpreter lock (GIL) has been enabled to load module 'capnp.lib.capnp'", so my understanding is when this test is run, the GIL is enabled and the test succeeds, except there is a spurious warning at the end which causes the test runner to fail.
So to me it seems like a good fix is disable the warning, since we know about it and are expecting it, and there not a good r
...
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2500447299)
> The correct fix would be to just require the GIL
That's right, the text of the warning (from #33582) states "The global interpreter lock (GIL) has been enabled to load module 'capnp.lib.capnp'", so my understanding is when this test is run, the GIL is enabled and the test succeeds, except there is a spurious warning at the end which causes the test runner to fail.
So to me it seems like a good fix is disable the warning, since we know about it and are expecting it, and there not a good r
...
🤔 janb84 reviewed a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3430058931)
Post merge ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969
To test this PR I made a dotnet wrapper for libbitcoinkernel and that went pretty smooth, no major issues.
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3430058931)
Post merge ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969
To test this PR I made a dotnet wrapper for libbitcoinkernel and that went pretty smooth, no major issues.
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2500462588)
On a system with 15 worker threads + main, it is likely that main will not be waiting much. The other 15 threads are busy setting newly fetch coins to ready so that the main thread can continuously read `true` for the ready flags.
On a system with only 3 worker threads + main, it is likely that the 3 worker threads will not be able to fetch and set ready coins fast enough where the main thread does not have to wait. For instance all 3 threads are fetching from disk, and the main thread reads th
...
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2500462588)
On a system with 15 worker threads + main, it is likely that main will not be waiting much. The other 15 threads are busy setting newly fetch coins to ready so that the main thread can continuously read `true` for the ready flags.
On a system with only 3 worker threads + main, it is likely that the 3 worker threads will not be able to fetch and set ready coins fast enough where the main thread does not have to wait. For instance all 3 threads are fetching from disk, and the main thread reads th
...
💬 maflcko commented on pull request "test: skip interface_ipc if python version is freethreaded and PYTHON_GIL=0 is not set":
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2500471963)
> > The correct fix would be to just require the GIL
>
> That's right, the text of the warning (from #33582) states "The global interpreter lock (GIL) has been enabled to load module 'capnp.lib.capnp'", so my understanding is when this test is run, the GIL is enabled and the test succeeds, except there is a spurious warning at the end which causes the test runner to fail.
Ok, this was not clear at all to me. It would be good to at least document, so that code-readers don't have to `git bla
...
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2500471963)
> > The correct fix would be to just require the GIL
>
> That's right, the text of the warning (from #33582) states "The global interpreter lock (GIL) has been enabled to load module 'capnp.lib.capnp'", so my understanding is when this test is run, the GIL is enabled and the test succeeds, except there is a spurious warning at the end which causes the test runner to fail.
Ok, this was not clear at all to me. It would be good to at least document, so that code-readers don't have to `git bla
...
💬 maflcko commented on pull request "util: Allow `Assert` (et al.) in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2500488831)
> There are 2 places in `validation.cpp` where `STR_INTERNAL_BUG` could be used too, but I suppose you didn't because it (afaik) requires us to abandon the compile time format string checking?
I think all format strings should be checked at compile time, even with `STR_INTERNAL_BUG`. I think `STR_INTERNAL_BUG` can't be used in the validation places, because `STR_INTERNAL_BUG` includes raw newlines and those are discouraged from logging. Though, it only happens on internal bugs anyway, so I ca
...
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2500488831)
> There are 2 places in `validation.cpp` where `STR_INTERNAL_BUG` could be used too, but I suppose you didn't because it (afaik) requires us to abandon the compile time format string checking?
I think all format strings should be checked at compile time, even with `STR_INTERNAL_BUG`. I think `STR_INTERNAL_BUG` can't be used in the validation places, because `STR_INTERNAL_BUG` includes raw newlines and those are discouraged from logging. Though, it only happens on internal bugs anyway, so I ca
...
💬 ajtowns commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3499059367)
> Doe this mean that, due to `AreInputsStandard`, even more UTXOs were soft-confiscated when #12460 was added?
I think that's pretty likely, but haven't confirmed. Should mean that bare multisigs are only spendable if any data is pushed as a fake un/compressed pubkey.
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3499059367)
> Doe this mean that, due to `AreInputsStandard`, even more UTXOs were soft-confiscated when #12460 was added?
I think that's pretty likely, but haven't confirmed. Should mean that bare multisigs are only spendable if any data is pushed as a fake un/compressed pubkey.
👍 hodlinator approved a pull request: "util: Allow `Assert` (et al.) in contexts without __func__"
(https://github.com/bitcoin/bitcoin/pull/33785#pullrequestreview-3430025702)
re-ACK fad6efd3bef1d123f806d492f019e29530b03a5e
2 added commits since previous review (https://github.com/bitcoin/bitcoin/pull/33785#pullrequestreview-3427192513):
* Remove no longer needed disabling of linter according to CI. 👍 (Re-touching `NONFATAL_UNREACHABLE` but no big deal).
* De-duplicate code by using `STR_INTERNAL_BUG()`.
(https://github.com/bitcoin/bitcoin/pull/33785#pullrequestreview-3430025702)
re-ACK fad6efd3bef1d123f806d492f019e29530b03a5e
2 added commits since previous review (https://github.com/bitcoin/bitcoin/pull/33785#pullrequestreview-3427192513):
* Remove no longer needed disabling of linter according to CI. 👍 (Re-touching `NONFATAL_UNREACHABLE` but no big deal).
* De-duplicate code by using `STR_INTERNAL_BUG()`.
💬 hodlinator commented on pull request "util: Allow `Assert` (et al.) in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2500429489)
nit: This is a case of clang-format failing to understand what is happening inside a macro.
https://releases.llvm.org/17.0.1/tools/clang/docs/ClangFormatStyleOptions.html#cpp11bracedliststyle
https://github.com/bitcoin/bitcoin/blob/fada379589a17e86396aa7c2ce458ff2ff602b84/src/.clang-format#L87
If I in the same commit change this to...
```C++
throw NonFatalCheckError{"Unreachable code reached (non-fatal)", std::source_location::current()}
```
...and also add...
```C++
void Func_NONFATAL_
...
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2500429489)
nit: This is a case of clang-format failing to understand what is happening inside a macro.
https://releases.llvm.org/17.0.1/tools/clang/docs/ClangFormatStyleOptions.html#cpp11bracedliststyle
https://github.com/bitcoin/bitcoin/blob/fada379589a17e86396aa7c2ce458ff2ff602b84/src/.clang-format#L87
If I in the same commit change this to...
```C++
throw NonFatalCheckError{"Unreachable code reached (non-fatal)", std::source_location::current()}
```
...and also add...
```C++
void Func_NONFATAL_
...
💬 flack commented on pull request "refactor: remove dead branches in `SingletonClusterImpl`":
(https://github.com/bitcoin/bitcoin/pull/33768#discussion_r2500676630)
this now always returns true. Is that needed for anything?
(https://github.com/bitcoin/bitcoin/pull/33768#discussion_r2500676630)
this now always returns true. Is that needed for anything?
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2500696484)
We discussed this out of band, here's the summary:
* the main thread is special because it has access to the dbcache, it can insert there without locking the cache.
* the threads each have their inputs now, each of which have a switch to signal when they've fetched the input and move on to the next
* the threads each compete for which input to fetch, marking them one-by-one as ready
* the main thread goes in order, spins until the next one is available, if it needs to wait, it does some fetc
...
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2500696484)
We discussed this out of band, here's the summary:
* the main thread is special because it has access to the dbcache, it can insert there without locking the cache.
* the threads each have their inputs now, each of which have a switch to signal when they've fetched the input and move on to the next
* the threads each compete for which input to fetch, marking them one-by-one as ready
* the main thread goes in order, spins until the next one is available, if it needs to wait, it does some fetc
...
💬 l0rinc commented on pull request "refactor: remove dead branches in `SingletonClusterImpl`":
(https://github.com/bitcoin/bitcoin/pull/33768#discussion_r2500745964)
Good question, I have already checked that, the sibling [GenericClusterImpl::Split](https://github.com/bitcoin/bitcoin/blob/master/src/txgraph.cpp#L1392) can return `false` which would [trigger deletion](https://github.com/bitcoin/bitcoin/blob/master/src/txgraph.cpp#L1773-L1777).
(https://github.com/bitcoin/bitcoin/pull/33768#discussion_r2500745964)
Good question, I have already checked that, the sibling [GenericClusterImpl::Split](https://github.com/bitcoin/bitcoin/blob/master/src/txgraph.cpp#L1392) can return `false` which would [trigger deletion](https://github.com/bitcoin/bitcoin/blob/master/src/txgraph.cpp#L1773-L1777).
💬 sipa commented on pull request "refactor: remove dead branches in `SingletonClusterImpl`":
(https://github.com/bitcoin/bitcoin/pull/33768#discussion_r2500769009)
Yeah. It's indeed the case that within `SingletonClusterImpl` it'll always return `true`, but this isn't the case for all possible `Cluster` implementations.
(https://github.com/bitcoin/bitcoin/pull/33768#discussion_r2500769009)
Yeah. It's indeed the case that within `SingletonClusterImpl` it'll always return `true`, but this isn't the case for all possible `Cluster` implementations.