💬 ryanofsky commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2132334798)
In commit "validation: in invalidateblock, calculate m_best_header right away" (809413fde6893e60078e02b401a9d98a3e341fc1)
It seems like it could be better to use CBlockIndexWorkComparator here instead of relying highpow_outofchain_headers multimap ordering when choosing m_best_header and two headers have the same mount of work because CBlockIndexWorkComparator should be more deterministic and consistent with surrounding code, while map order will depend on iteration order of the `m_block_inde
...
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2132334798)
In commit "validation: in invalidateblock, calculate m_best_header right away" (809413fde6893e60078e02b401a9d98a3e341fc1)
It seems like it could be better to use CBlockIndexWorkComparator here instead of relying highpow_outofchain_headers multimap ordering when choosing m_best_header and two headers have the same mount of work because CBlockIndexWorkComparator should be more deterministic and consistent with surrounding code, while map order will depend on iteration order of the `m_block_inde
...
💬 ryanofsky commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2132373845)
In commit "doc: Improve m_best_header documentation" (fdcdc2cdeadb5d36076f1b94b54261e22e031354)
It seems like it would be nice to use CBlockIndexWorkComparator consistently for comparisons, but since it's probably beyond scope of this PR, it's good to have this documentation comment mentioning the lack of consistency.
Just in case its useful for future reference, the post describing the current situation in the most detail is https://github.com/bitcoin/bitcoin/pull/31405#discussion_r212978
...
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2132373845)
In commit "doc: Improve m_best_header documentation" (fdcdc2cdeadb5d36076f1b94b54261e22e031354)
It seems like it would be nice to use CBlockIndexWorkComparator consistently for comparisons, but since it's probably beyond scope of this PR, it's good to have this documentation comment mentioning the lack of consistency.
Just in case its useful for future reference, the post describing the current situation in the most detail is https://github.com/bitcoin/bitcoin/pull/31405#discussion_r212978
...
💬 ryanofsky commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2132231220)
re: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121892715
In commit "validation: in invalidateblock, mark children as invalid right away" (328345d571d9b86af54e915b30183b91d28cab2f)
Thanks for the new comment. Now that there's an explanation directly in the code, I think it would be good to replace the text "Entries from..." paragraph with a simple statement that the commit doesn't change behavior, since the paragraph only repeats the code comment out of context.
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2132231220)
re: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2121892715
In commit "validation: in invalidateblock, mark children as invalid right away" (328345d571d9b86af54e915b30183b91d28cab2f)
Thanks for the new comment. Now that there's an explanation directly in the code, I think it would be good to replace the text "Entries from..." paragraph with a simple statement that the commit doesn't change behavior, since the paragraph only repeats the code comment out of context.
💬 hebasto commented on issue "depends: OpenBSD (aarch64) needs gcc (instruction) for libevent":
(https://github.com/bitcoin/bitcoin/issues/32691#issuecomment-2949641365)
@Sjors
Could you please execute the following `CMakeLists.txt` on your machine:
```cmake
cmake_minimum_required(VERSION 3.31)
project(openbsd_cc C)
```
and submit a bug report to https://gitlab.kitware.com/cmake/cmake?
(https://github.com/bitcoin/bitcoin/issues/32691#issuecomment-2949641365)
@Sjors
Could you please execute the following `CMakeLists.txt` on your machine:
```cmake
cmake_minimum_required(VERSION 3.31)
project(openbsd_cc C)
```
and submit a bug report to https://gitlab.kitware.com/cmake/cmake?
💬 jmatcho commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2949643302)
@petertodd you’re loading up with the logical fallacies which demonstrates the need for politics in the absence of a genuine need to change the code at this time.
> Here, we have good consensus among reasonable devs that actively contribute to Bitcoin Core that this change is a good idea.
Ignoring however you choose to define “good consensus” and “reasonable”, you’re employing several logical fallacies: Appeal to Authority, Appeal to Popularity, Appeal to Tradition, and I’m sure some ot
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2949643302)
@petertodd you’re loading up with the logical fallacies which demonstrates the need for politics in the absence of a genuine need to change the code at this time.
> Here, we have good consensus among reasonable devs that actively contribute to Bitcoin Core that this change is a good idea.
Ignoring however you choose to define “good consensus” and “reasonable”, you’re employing several logical fallacies: Appeal to Authority, Appeal to Popularity, Appeal to Tradition, and I’m sure some ot
...
👍 ryanofsky approved a pull request: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2905300599)
Code review ACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1 with only minor code & comment updates
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2905300599)
Code review ACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1 with only minor code & comment updates
💬 Sjors commented on pull request "depends: fix multiprocess build on OpenBSD (apply capnp patch, correct SHA256SUM command)":
(https://github.com/bitcoin/bitcoin/pull/32690#issuecomment-2949660658)
ACK 8713e8060d504f561fed705b4aa5af7b96c36e75
Switched to an x86_64 based VM and it builds fine. I also ran the Bitcoin Core test suite.
Can you add a note to the OpenBSD instruction in `depends/README.md` to also install `cmake`?
(https://github.com/bitcoin/bitcoin/pull/32690#issuecomment-2949660658)
ACK 8713e8060d504f561fed705b4aa5af7b96c36e75
Switched to an x86_64 based VM and it builds fine. I also ran the Bitcoin Core test suite.
Can you add a note to the OpenBSD instruction in `depends/README.md` to also install `cmake`?
💬 hebasto commented on pull request "depends: fix multiprocess build on OpenBSD (apply capnp patch, correct SHA256SUM command)":
(https://github.com/bitcoin/bitcoin/pull/32690#discussion_r2132436423)
I was blind. Sorry for the noise.
(https://github.com/bitcoin/bitcoin/pull/32690#discussion_r2132436423)
I was blind. Sorry for the noise.
💬 theStack commented on pull request "depends: fix multiprocess build on OpenBSD (apply capnp patch, correct SHA256SUM command)":
(https://github.com/bitcoin/bitcoin/pull/32690#issuecomment-2949708227)
> Why [8713e80](https://github.com/bitcoin/bitcoin/commit/8713e8060d504f561fed705b4aa5af7b96c36e75) is necessary for libmultiprocess, but not for other packages?
The libmultiprocess package is the only one which sets the [`$(package)_local_dir`](https://github.com/bitcoin/bitcoin/blob/ae024137bda9fe189f4e7ccf26dbaffd44cbbeb6/depends/packages/native_libmultiprocess.mk#L2) variable, causing execution of the [`fetch_local_dir_sha256` shell script](https://github.com/bitcoin/bitcoin/blob/ae024137
...
(https://github.com/bitcoin/bitcoin/pull/32690#issuecomment-2949708227)
> Why [8713e80](https://github.com/bitcoin/bitcoin/commit/8713e8060d504f561fed705b4aa5af7b96c36e75) is necessary for libmultiprocess, but not for other packages?
The libmultiprocess package is the only one which sets the [`$(package)_local_dir`](https://github.com/bitcoin/bitcoin/blob/ae024137bda9fe189f4e7ccf26dbaffd44cbbeb6/depends/packages/native_libmultiprocess.mk#L2) variable, causing execution of the [`fetch_local_dir_sha256` shell script](https://github.com/bitcoin/bitcoin/blob/ae024137
...
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-2949722160)
All comments addressed, ready for review. Will write up some notes for a review club soon.
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-2949722160)
All comments addressed, ready for review. Will write up some notes for a review club soon.
💬 HowHsu commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-2949724891)
> I think up to `nCores` makes sense, and we need to be sure that that value is an actual reflection of hardware concurrency on all platforms. It may be worth benchmarking to see if there is actually a gain above certain values still (because thread contention will start to become more and more noticeable, which may at some point overtake the gains from more parallellism).
There is a master worker, plus `nCores-1` normal workers, so setting it to `nCores` actually allows `nCores+1` worker thr
...
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-2949724891)
> I think up to `nCores` makes sense, and we need to be sure that that value is an actual reflection of hardware concurrency on all platforms. It may be worth benchmarking to see if there is actually a gain above certain values still (because thread contention will start to become more and more noticeable, which may at some point overtake the gains from more parallellism).
There is a master worker, plus `nCores-1` normal workers, so setting it to `nCores` actually allows `nCores+1` worker thr
...
💬 theStack commented on pull request "depends: fix multiprocess build on OpenBSD (apply capnp patch, correct SHA256SUM command)":
(https://github.com/bitcoin/bitcoin/pull/32690#issuecomment-2949729126)
@Sjors:
> Can you add a note to the OpenBSD instruction in `depends/README.md` to also install `cmake`?
Seems like material for a different PR, considering this affects other BSDs as well, and at first glance it seems there are even other packages missing, like e.g. `curl` (which AFAIR, is also not part of OpenBSD's base system). Will open a follow-up later.
(https://github.com/bitcoin/bitcoin/pull/32690#issuecomment-2949729126)
@Sjors:
> Can you add a note to the OpenBSD instruction in `depends/README.md` to also install `cmake`?
Seems like material for a different PR, considering this affects other BSDs as well, and at first glance it seems there are even other packages missing, like e.g. `curl` (which AFAIR, is also not part of OpenBSD's base system). Will open a follow-up later.
💬 Sjors commented on issue "depends: OpenBSD (aarch64) needs gcc (instruction) for libevent":
(https://github.com/bitcoin/bitcoin/issues/32691#issuecomment-2949732117)
```
$ cmake -B build
-- The C compiler identification is Clang 16.0.6
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Configuring done (1.5s)
-- Generating done (0.0s)
-- Build files have been written to: /home/sjors/src/debug/build
```
The output is identical to that on x86 machine, so I'm not sure what the bug is?
(https://github.com/bitcoin/bitcoin/issues/32691#issuecomment-2949732117)
```
$ cmake -B build
-- The C compiler identification is Clang 16.0.6
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Configuring done (1.5s)
-- Generating done (0.0s)
-- Build files have been written to: /home/sjors/src/debug/build
```
The output is identical to that on x86 machine, so I'm not sure what the bug is?
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2132459455)
> I don't see how this affects any maps, though? I think it just affects the messages when we stop and restart logging?
Oh, I think I mistakenly thought that `function_name` was included in the map somehow or used when hashing `std::source_location`, but I see that's not actually the case.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2132459455)
> I don't see how this affects any maps, though? I think it just affects the messages when we stop and restart logging?
Oh, I think I mistakenly thought that `function_name` was included in the map somehow or used when hashing `std::source_location`, but I see that's not actually the case.
💬 sipa commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-2949745049)
> There is a master worker, plus nCores-1 normal workers, so setting it to nCores actually allows nCores+1 worker threads.
That is incorrect. With `-par=N`, there is 1 master plus `N-1` workers, which both perform script validation work, but the master only joins the other threads when it is done with other work. If you have N perfectly parallel cores, and nothing else running on your machine, `par=N` is expected, so the software should allow up to that, if there is no reason to assume that t
...
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-2949745049)
> There is a master worker, plus nCores-1 normal workers, so setting it to nCores actually allows nCores+1 worker threads.
That is incorrect. With `-par=N`, there is 1 master plus `N-1` workers, which both perform script validation work, but the master only joins the other threads when it is done with other work. If you have N perfectly parallel cores, and nothing else running on your machine, `par=N` is expected, so the software should allow up to that, if there is no reason to assume that t
...
💬 sipa commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#discussion_r2132479314)
Don't do this. Upper-case variable names are for constants, and this is no longer a constant. If there is reason for it to be a variable, the constant and its name should be removed, and the code using it should be adapted to compute it on the fly.
(https://github.com/bitcoin/bitcoin/pull/32692#discussion_r2132479314)
Don't do this. Upper-case variable names are for constants, and this is no longer a constant. If there is reason for it to be a variable, the constant and its name should be removed, and the code using it should be adapted to compute it on the fly.
💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2132482525)
> Hmm, is there a reason why blockundo takes only 1.6ms in total end-to-end when spenttxouts takes 4.6ms in total end-to-end, where about 2.6ms happen on the server (https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2109843001). So with spenttxouts, the client spends about 2ms with 18% CPU and with blockundo the client spends about .8ms with 58% CPU.
I would like to note that https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2109843001 was benchmarked using [ApacheBench](http
...
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2132482525)
> Hmm, is there a reason why blockundo takes only 1.6ms in total end-to-end when spenttxouts takes 4.6ms in total end-to-end, where about 2.6ms happen on the server (https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2109843001). So with spenttxouts, the client spends about 2ms with 18% CPU and with blockundo the client spends about .8ms with 58% CPU.
I would like to note that https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2109843001 was benchmarked using [ApacheBench](http
...
💬 HowHsu commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-2949812407)
> > There is a master worker, plus nCores-1 normal workers, so setting it to nCores actually allows nCores+1 worker threads.
>
> That is incorrect. With `-par=N`, there is 1 master plus `N-1` workers, which both perform script validation work, but the master only joins the other threads when it is done with other work. If you have N perfectly parallel cores, and nothing else running on your machine, `par=N` is expected, so the software should allow up to that, if there is no reason to assume
...
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-2949812407)
> > There is a master worker, plus nCores-1 normal workers, so setting it to nCores actually allows nCores+1 worker threads.
>
> That is incorrect. With `-par=N`, there is 1 master plus `N-1` workers, which both perform script validation work, but the master only joins the other threads when it is done with other work. If you have N perfectly parallel cores, and nothing else running on your machine, `par=N` is expected, so the software should allow up to that, if there is no reason to assume
...
💬 sipa commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-2949817904)
> I think par=N means N workers, plus one master worker which is actually the main bitcoind thread itself.
That's not correct. The number of worker threads is one less than the argument to `-par`: https://github.com/bitcoin/bitcoin/blob/v29.0/src/node/chainstatemanager_args.cpp#L62.
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-2949817904)
> I think par=N means N workers, plus one master worker which is actually the main bitcoind thread itself.
That's not correct. The number of worker threads is one less than the argument to `-par`: https://github.com/bitcoin/bitcoin/blob/v29.0/src/node/chainstatemanager_args.cpp#L62.
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2132510030)
I'm a little unsure about adding rate-limiting to `LogPrintLevel` as then if a user runs with -debug, they could potentially hit the rate-limiting messages via `LogDebug` or by some of the existing `LogPrintLevel` calls. I think it is in the user's best interest to have rate-limiting applied to all public macros like you suggested, but I also don't want to potentially confuse users as to why their debug.log is incomplete due to rate-limiting when they are just running with `-debug`.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2132510030)
I'm a little unsure about adding rate-limiting to `LogPrintLevel` as then if a user runs with -debug, they could potentially hit the rate-limiting messages via `LogDebug` or by some of the existing `LogPrintLevel` calls. I think it is in the user's best interest to have rate-limiting applied to all public macros like you suggested, but I also don't want to potentially confuse users as to why their debug.log is incomplete due to rate-limiting when they are just running with `-debug`.