💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574936137)
If there is a missing new line github will show a red arrow at the end of the file.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574936137)
If there is a missing new line github will show a red arrow at the end of the file.
💬 mdabdulkaderhossain144-ux commented on something "":
(https://github.com/bitcoin/bitcoin/commit/2e27bd9c3af91eb9fcc626fe65d065df0a80974d#commitcomment-171659028)
0xfdDB0e3EDbc6ffC1cF63f0c47d0Db9c9EA7EC671
(https://github.com/bitcoin/bitcoin/commit/2e27bd9c3af91eb9fcc626fe65d065df0a80974d#commitcomment-171659028)
0xfdDB0e3EDbc6ffC1cF63f0c47d0Db9c9EA7EC671
💬 mdabdulkaderhossain144-ux commented on something "":
(https://github.com/bitcoin/bitcoin/commit/2e27bd9c3af91eb9fcc626fe65d065df0a80974d#commitcomment-171659046)
2e27bd9c3af91eb9fcc626fe65d065df0a80974d
(https://github.com/bitcoin/bitcoin/commit/2e27bd9c3af91eb9fcc626fe65d065df0a80974d#commitcomment-171659046)
2e27bd9c3af91eb9fcc626fe65d065df0a80974d
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574956644)
> isn't this too small for our purposes?
I don't see why we need any randomness or larger values for these?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574956644)
> isn't this too small for our purposes?
I don't see why we need any randomness or larger values for these?
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574968743)
Hmm not sure how we'd test this with a unit test. It surely gets exercised by fuzzing though.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574968743)
Hmm not sure how we'd test this with a unit test. It surely gets exercised by fuzzing though.
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574970212)
It gets called by functional tests though.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574970212)
It gets called by functional tests though.
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574973805)
> before/after
Not sure there are any before results we can use for this.
> can you reproduce 4 threads saturating the parallelism factor with it?
How would I know if I do that?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574973805)
> before/after
Not sure there are any before results we can use for this.
> can you reproduce 4 threads saturating the parallelism factor with it?
How would I know if I do that?
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574979427)
That's more an implementation detail that can be found by reading the header file, no?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574979427)
That's more an implementation detail that can be found by reading the header file, no?
💬 sedited commented on pull request "refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-3593597789)
Looking at this patch, I am asking myself why we're keeping the single threaded / no checkqueue case in the first place. This seems to introduce a vector of jobs for both cases now, so maybe we could just create a queue without any workers?
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-3593597789)
Looking at this patch, I am asking myself why we're keeping the single threaded / no checkqueue case in the first place. This seems to introduce a vector of jobs for both cases now, so maybe we could just create a queue without any workers?
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574980697)
I would prefer to split out the implementation, the tests, the fuzz harness... But you prefer those all be in the same commit?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574980697)
I would prefer to split out the implementation, the tests, the fuzz harness... But you prefer those all be in the same commit?
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574982084)
> (i.e. to stop propagation to disk)
The parent could be called, but this is faster since we skip calling `ReallocateCache`.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574982084)
> (i.e. to stop propagation to disk)
The parent could be called, but this is faster since we skip calling `ReallocateCache`.
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574988562)
I'm not sure if we need all these deletes here? What are we gaining from this?
We need to have move assignment for it to compile. It can't automatically move the `atomic_flag`. So, it doesn't really matter if it gets called or not, we still need to define it. And, since it only happens before we start doing work, might as well make it simple and not bother moving the other fields. I don't think we need to bother reserving since we keep the capacity over many blocks. The looping is just extra
...
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574988562)
I'm not sure if we need all these deletes here? What are we gaining from this?
We need to have move assignment for it to compile. It can't automatically move the `atomic_flag`. So, it doesn't really matter if it gets called or not, we still need to define it. And, since it only happens before we start doing work, might as well make it simple and not bother moving the other fields. I don't think we need to bother reserving since we keep the capacity over many blocks. The looping is just extra
...
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574995185)
It doesn't simulate going to disk. It simulates not setting the coin in `ProcessInputInBackground` even though the base has it. Then the `if (ret->second.coin.IsSpent()) [[unlikely]] {` branch is executed in `FetchCoin` and the coin is fetched from base via `GetCoinWithoutMutating`.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2574995185)
It doesn't simulate going to disk. It simulates not setting the coin in `ProcessInputInBackground` even though the base has it. Then the `if (ret->second.coin.IsSpent()) [[unlikely]] {` branch is executed in `FetchCoin` and the coin is fetched from base via `GetCoinWithoutMutating`.
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2575017536)
> would basically pass (but hang) if we forget to Reset
A hanging test is treated as failure in the CI. I don't think it's necessary to do anything else here.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2575017536)
> would basically pass (but hang) if we forget to Reset
A hanging test is treated as failure in the CI. I don't think it's necessary to do anything else here.
📝 hebasto opened a pull request: "depends, doc: Add `tcl` as build dependency for `sqlite` package"
(https://github.com/bitcoin/bitcoin/pull/33975)
Since https://github.com/bitcoin/bitcoin/pull/32655, the `sqlite` package has relied on a native C compiler being incidentally available on the system under the default names. However, this is not always the case. For example, on Ubuntu 24.04:
```
$ gmake -C depends sqlite CC=gcc-14 CXX=g++-14
gmake: Entering directory '/bitcoin/depends'
Configuring sqlite...
No installed jimsh or tclsh, building local bootstrap jimsh0
No working C compiler found. Tried cc and gcc.
gmake: *** [funcs.mk:34
...
(https://github.com/bitcoin/bitcoin/pull/33975)
Since https://github.com/bitcoin/bitcoin/pull/32655, the `sqlite` package has relied on a native C compiler being incidentally available on the system under the default names. However, this is not always the case. For example, on Ubuntu 24.04:
```
$ gmake -C depends sqlite CC=gcc-14 CXX=g++-14
gmake: Entering directory '/bitcoin/depends'
Configuring sqlite...
No installed jimsh or tclsh, building local bootstrap jimsh0
No working C compiler found. Tried cc and gcc.
gmake: *** [funcs.mk:34
...
💬 hebasto commented on pull request "depends, doc: Add `tcl` as build dependency for `sqlite` package":
(https://github.com/bitcoin/bitcoin/pull/33975#discussion_r2575046363)
FWIW, [`jimsh`](https://packages.ubuntu.com/noble/jimsh) works as well.
`tcl` is chosen for consistency with the Guix script.
(https://github.com/bitcoin/bitcoin/pull/33975#discussion_r2575046363)
FWIW, [`jimsh`](https://packages.ubuntu.com/noble/jimsh) works as well.
`tcl` is chosen for consistency with the Guix script.
📝 SmartDever02 opened a pull request: "net: Complete net/net_processing split refactoring"
(https://github.com/bitcoin/bitcoin/pull/33976)
```
Completes the net split refactoring (#33958) by cleanly separating `net` and `net_processing` layers through four incremental phases. This refactoring addresses the tight coupling between network connection management (`CConnman`/`CNode`) and peer message processing (`PeerManager`), enabling better code organization and maintainability.
## Phase 1: Move connection state from CNode to Peer
Moved connection-specific state from `CNode` to the `Peer` structure to reduce direct dependencie
...
(https://github.com/bitcoin/bitcoin/pull/33976)
```
Completes the net split refactoring (#33958) by cleanly separating `net` and `net_processing` layers through four incremental phases. This refactoring addresses the tight coupling between network connection management (`CConnman`/`CNode`) and peer message processing (`PeerManager`), enabling better code organization and maintainability.
## Phase 1: Move connection state from CNode to Peer
Moved connection-specific state from `CNode` to the `Peer` structure to reduce direct dependencie
...
💬 pinheadmz commented on pull request "net: Complete net/net_processing split refactoring":
(https://github.com/bitcoin/bitcoin/pull/33976#issuecomment-3594200550)
You closed that entire issue? All four of these "phases"... in two commits? You are ranked #2 on Gittensor, what is that
(https://github.com/bitcoin/bitcoin/pull/33976#issuecomment-3594200550)
You closed that entire issue? All four of these "phases"... in two commits? You are ranked #2 on Gittensor, what is that
💬 pinheadmz commented on pull request "net: Complete net/net_processing split refactoring":
(https://github.com/bitcoin/bitcoin/pull/33976#issuecomment-3594207361)
Did you get all these compile errors when you built this code locally for testing as well? Or is it just me
```
In file included from /home/pinheadmz/Desktop/work/bitcoin/src/netmessagemaker.h:9,
from /home/pinheadmz/Desktop/work/bitcoin/src/test/util/net.h:9,
from /home/pinheadmz/Desktop/work/bitcoin/src/test/util/net.cpp:5:
/home/pinheadmz/Desktop/work/bitcoin/src/net.h:676:1: error: expected class-name before ‘{’ token
676 | {
| ^
/home/pinh
...
(https://github.com/bitcoin/bitcoin/pull/33976#issuecomment-3594207361)
Did you get all these compile errors when you built this code locally for testing as well? Or is it just me
```
In file included from /home/pinheadmz/Desktop/work/bitcoin/src/netmessagemaker.h:9,
from /home/pinheadmz/Desktop/work/bitcoin/src/test/util/net.h:9,
from /home/pinheadmz/Desktop/work/bitcoin/src/test/util/net.cpp:5:
/home/pinheadmz/Desktop/work/bitcoin/src/net.h:676:1: error: expected class-name before ‘{’ token
676 | {
| ^
/home/pinh
...
📝 vostrnad opened a pull request: "script: Remove dead code from OP_CHECKMULTISIG"
(https://github.com/bitcoin/bitcoin/pull/33977)
The stack size check before the dummy element check in OP_CHECKMULTISIG is redundant. The stack is previously validated to have least *i* elements (where *i* is number of pubkeys + number of signatures + 3), and then *i* - 1 elements are popped off the stack, leaving at least one element still in.
The redundant check was added in #3843 (commit 6380180821917c22ecfd89128ee60aae6f4cac33). There it's easy to see why it's redundant – it's inserted into a code path where another `popstack` would pr
...
(https://github.com/bitcoin/bitcoin/pull/33977)
The stack size check before the dummy element check in OP_CHECKMULTISIG is redundant. The stack is previously validated to have least *i* elements (where *i* is number of pubkeys + number of signatures + 3), and then *i* - 1 elements are popped off the stack, leaving at least one element still in.
The redundant check was added in #3843 (commit 6380180821917c22ecfd89128ee60aae6f4cac33). There it's easy to see why it's redundant – it's inserted into a code path where another `popstack` would pr
...