💬 HowHsu commented on pull request "checkqueue: implement a new scriptcheck worker pool with atomic variables":
(https://github.com/bitcoin/bitcoin/pull/32791#issuecomment-3016695705)
> hmmm...Ok, I realized that this may only be suitable for when all the preout Coin are in CCoinsViewCache, otherwise the `Add()` is not fast due to I/O there.
With #31132 , this one makes sense again, need to test them together in all-cache-miss case
(https://github.com/bitcoin/bitcoin/pull/32791#issuecomment-3016695705)
> hmmm...Ok, I realized that this may only be suitable for when all the preout Coin are in CCoinsViewCache, otherwise the `Add()` is not fast due to I/O there.
With #31132 , this one makes sense again, need to test them together in all-cache-miss case
💬 HowHsu commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-3016699198)
I post a new scriptcheck pool implementation #32791 with atomic variables rather than mutex to reduce thread sync contention. That achieves very good performance improvement. But the idea is based on that `the adding period is very short compared to the script verification period`, so it has to be with PR #31132. @sipa and others, welcome to take a look.
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-3016699198)
I post a new scriptcheck pool implementation #32791 with atomic variables rather than mutex to reduce thread sync contention. That achieves very good performance improvement. But the idea is based on that `the adding period is very short compared to the script verification period`, so it has to be with PR #31132. @sipa and others, welcome to take a look.
📝 HowHsu reopened a pull request: "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1"
(https://github.com/bitcoin/bitcoin/pull/32692)
A fixed MAX_SCRIPTCHECK_THREADS value is not flexible for users to leverage their cpu resources, and a value large than nCores - 1 doesn't make sense since it only adds some context switch overhead. Set it to nCores - 1. Assumption: A user who sets the number of script verification workers is aware of how this affects the system performance, otherwise he/she leaves it as default (which is 0)
(https://github.com/bitcoin/bitcoin/pull/32692)
A fixed MAX_SCRIPTCHECK_THREADS value is not flexible for users to leverage their cpu resources, and a value large than nCores - 1 doesn't make sense since it only adds some context switch overhead. Set it to nCores - 1. Assumption: A user who sets the number of script verification workers is aware of how this affects the system performance, otherwise he/she leaves it as default (which is 0)
💬 HowHsu commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-3016707658)
> Maybe this is something best solved by runtime benchmarking? (Or perhaps just at startup, since early blocks may not give us the parallelization we need for it)
I think we can do it so, binary search the best worker thread number with bencharking on some standard test data.
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-3016707658)
> Maybe this is something best solved by runtime benchmarking? (Or perhaps just at startup, since early blocks may not give us the parallelization we need for it)
I think we can do it so, binary search the best worker thread number with bencharking on some standard test data.
💬 yuvicc commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3016746882)
Concept ACK
```
./build/bin/bitcoin-cli -named finalizepsbt cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O=g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA=
error code: -8
error message:
Unknown named parameter cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/
...
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3016746882)
Concept ACK
```
./build/bin/bitcoin-cli -named finalizepsbt cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O=g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA=
error code: -8
error message:
Unknown named parameter cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/
...
💬 yuvicc commented on pull request "bench: replace embedded raw block with configurable block generator":
(https://github.com/bitcoin/bitcoin/pull/32554#issuecomment-3016802388)
Concept ACK
This one is pretty useful as I can test with up-to date test_data.
(https://github.com/bitcoin/bitcoin/pull/32554#issuecomment-3016802388)
Concept ACK
This one is pretty useful as I can test with up-to date test_data.
💬 dergoegge commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3016809091)
I fuzzed this branch (https://github.com/bitcoin/bitcoin/pull/28676/commits/3bfaedbd7d9d6e71e9df03fbbb51a33c6184cca6) with [fuzzamoto](https://github.com/dergoegge/fuzzamoto) and it found an assertion crash in `CTxMemPool::check`.
```
2025-06-29T15:20:42Z (mocktime: 2031-04-03T07:09:03Z) [bench] - Disconnect block: 0.72ms
2025-06-29T15:20:42Z (mocktime: 2031-04-03T07:09:03Z) [prune] basic block filter index prune lock moved back to 200
2025-06-29T15:20:42Z (mocktime: 2031-04-03T07:09:03Z)
...
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3016809091)
I fuzzed this branch (https://github.com/bitcoin/bitcoin/pull/28676/commits/3bfaedbd7d9d6e71e9df03fbbb51a33c6184cca6) with [fuzzamoto](https://github.com/dergoegge/fuzzamoto) and it found an assertion crash in `CTxMemPool::check`.
```
2025-06-29T15:20:42Z (mocktime: 2031-04-03T07:09:03Z) [bench] - Disconnect block: 0.72ms
2025-06-29T15:20:42Z (mocktime: 2031-04-03T07:09:03Z) [prune] basic block filter index prune lock moved back to 200
2025-06-29T15:20:42Z (mocktime: 2031-04-03T07:09:03Z)
...
🤔 BrandonOdiwuor reviewed a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-2969336275)
Tested ACK e246741db1f2f056fac32d0f3967f55306e78b49
Confirmed that commit 59fcefcd1f490f8ae276150421cbd895a10f3d4a correctly handles named argument parsing for parameters containing = characters—an issue currently reproducible on master.
<commit: 59fcefcd1f490f8ae276150421cbd895a10f3d4a>
<img width="1512" alt="Screenshot 2025-06-29 at 19 49 25" src="https://github.com/user-attachments/assets/fc4e2230-453d-4691-9ff5-db980693bc85" />
<branch: master>
<img width="1512" alt="Screenshot
...
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-2969336275)
Tested ACK e246741db1f2f056fac32d0f3967f55306e78b49
Confirmed that commit 59fcefcd1f490f8ae276150421cbd895a10f3d4a correctly handles named argument parsing for parameters containing = characters—an issue currently reproducible on master.
<commit: 59fcefcd1f490f8ae276150421cbd895a10f3d4a>
<img width="1512" alt="Screenshot 2025-06-29 at 19 49 25" src="https://github.com/user-attachments/assets/fc4e2230-453d-4691-9ff5-db980693bc85" />
<branch: master>
<img width="1512" alt="Screenshot
...
📝 jlopp opened a pull request: "add more bad p2p ports"
(https://github.com/bitcoin/bitcoin/pull/32826)
Add a few more ports used by extremely well adopted services that require authentication and really ought not be used by bitcoin nodes for p2p traffic.
(https://github.com/bitcoin/bitcoin/pull/32826)
Add a few more ports used by extremely well adopted services that require authentication and really ought not be used by bitcoin nodes for p2p traffic.
📝 l0rinc opened a pull request: "mempool: Avoid needless vtx iteration in `removeForBlock` during IBD"
(https://github.com/bitcoin/bitcoin/pull/32827)
During Initial Block Download, the mempool is known to be empty, but `CTxMemPool::removeForBlock` is still called for every connected block where we:
* iterate over every transaction in the block even though none will be found in the empty `mapTx`, always leaving `txs_removed_for_block` empty...
* which is pre-allocated regardless with 40 bytes * vtx.size(), even though it will always remain empty.
Similarly to https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2140691354, this chang
...
(https://github.com/bitcoin/bitcoin/pull/32827)
During Initial Block Download, the mempool is known to be empty, but `CTxMemPool::removeForBlock` is still called for every connected block where we:
* iterate over every transaction in the block even though none will be found in the empty `mapTx`, always leaving `txs_removed_for_block` empty...
* which is pre-allocated regardless with 40 bytes * vtx.size(), even though it will always remain empty.
Similarly to https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2140691354, this chang
...
💬 l0rinc commented on pull request "p2p: avoid traversing blocks (twice) during IBD":
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2173852408)
Opened it separately in https://github.com/bitcoin/bitcoin/pull/32827, please resolve this comment
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2173852408)
Opened it separately in https://github.com/bitcoin/bitcoin/pull/32827, please resolve this comment
📝 l0rinc converted_to_draft a pull request: "mempool: Avoid needless vtx iteration in `removeForBlock` during IBD"
(https://github.com/bitcoin/bitcoin/pull/32827)
During Initial Block Download, the mempool is known to be empty, but `CTxMemPool::removeForBlock` is still called for every connected block where we:
* iterate over every transaction in the block even though none will be found in the empty `mapTx`, always leaving `txs_removed_for_block` empty...
* which is pre-allocated regardless with 40 bytes * vtx.size(), even though it will always remain empty.
Similarly to https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2140691354, this chang
...
(https://github.com/bitcoin/bitcoin/pull/32827)
During Initial Block Download, the mempool is known to be empty, but `CTxMemPool::removeForBlock` is still called for every connected block where we:
* iterate over every transaction in the block even though none will be found in the empty `mapTx`, always leaving `txs_removed_for_block` empty...
* which is pre-allocated regardless with 40 bytes * vtx.size(), even though it will always remain empty.
Similarly to https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2140691354, this chang
...
💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2173853678)
I think `COMPONENT Kernel` should become `COMPONENT libbitcoinkernel`. Noticed when running `cmake --install build --component libbitcoinkernel` the header file wasn't being installed.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2173853678)
I think `COMPONENT Kernel` should become `COMPONENT libbitcoinkernel`. Noticed when running `cmake --install build --component libbitcoinkernel` the header file wasn't being installed.
👍 l0rinc approved a pull request: "p2p: add more bad ports"
(https://github.com/bitcoin/bitcoin/pull/32826#pullrequestreview-2969371289)
ACK 23a7807ec6d35d3ef5b65e4abc3b3841e84b702e
Verified that these are indeed the default ports and that the docs match the code.
Have a miner test suggestion, LGTM otherwise.
(https://github.com/bitcoin/bitcoin/pull/32826#pullrequestreview-2969371289)
ACK 23a7807ec6d35d3ef5b65e4abc3b3841e84b702e
Verified that these are indeed the default ports and that the docs match the code.
Have a miner test suggestion, LGTM otherwise.
💬 l0rinc commented on pull request "p2p: add more bad ports":
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2173855831)
instead of updating the comment to match the code (which is the ultimate source of truth anyway), seems like a good opportunity to remove the duplication of the code below - which is already trivial, no need for a dead comment that doesn't add any new info.
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2173855831)
instead of updating the comment to match the code (which is the ultimate source of truth anyway), seems like a good opportunity to remove the duplication of the code below - which is already trivial, no need for a dead comment that doesn't add any new info.
💬 l0rinc commented on pull request "p2p: add more bad ports":
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2173861432)
I was wondering if we could do a more functional approach here, since we already have a few `count_if` instances - not sure it's better or not, will let you decide, please resolve if you don't think it's a good alternative (assuming integer promotion for max + 1):
```suggestion
using namespace std::ranges;
auto ports{views::iota(1, std::numeric_limits<uint16_t>::max() + 1)};
BOOST_CHECK_EQUAL(count_if(ports, IsBadPort), 85);
```
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2173861432)
I was wondering if we could do a more functional approach here, since we already have a few `count_if` instances - not sure it's better or not, will let you decide, please resolve if you don't think it's a good alternative (assuming integer promotion for max + 1):
```suggestion
using namespace std::ranges;
auto ports{views::iota(1, std::numeric_limits<uint16_t>::max() + 1)};
BOOST_CHECK_EQUAL(count_if(ports, IsBadPort), 85);
```
💬 l0rinc commented on pull request "p2p: add more bad ports":
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2173856627)
👍 https://dev.mysql.com/doc/mysql-port-reference/en/mysql-port-reference-tables.html#GUID-65C1FF7E-5357-4E58-8D68-A0C3D24C0832__MYSQL-CLIENT-SERVER-PORTS
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2173856627)
👍 https://dev.mysql.com/doc/mysql-port-reference/en/mysql-port-reference-tables.html#GUID-65C1FF7E-5357-4E58-8D68-A0C3D24C0832__MYSQL-CLIENT-SERVER-PORTS
💬 l0rinc commented on pull request "p2p: add more bad ports":
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2173856757)
👍 https://www.datacentreplus.co.uk/support/knowledge-base/changing-the-rdp-port-on-a-windows-server
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2173856757)
👍 https://www.datacentreplus.co.uk/support/knowledge-base/changing-the-rdp-port-on-a-windows-server
💬 l0rinc commented on pull request "p2p: add more bad ports":
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2173857541)
👍 https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/6/html/deployment_guide/s1-connecting-to-vnc-server#sec-Configuring_the_Firewall_for_VNC
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2173857541)
👍 https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/6/html/deployment_guide/s1-connecting-to-vnc-server#sec-Configuring_the_Firewall_for_VNC
💬 l0rinc commented on pull request "p2p: add more bad ports":
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2173857685)
👍 https://www.mongodb.com/docs/manual/reference/default-mongodb-port
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2173857685)
👍 https://www.mongodb.com/docs/manual/reference/default-mongodb-port