💬 stickies-v commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347370849)
Is there a reason you’re using `kernel_cache_sizes.coins` instead of just inspecting `-dbcache`? Using the latter is more in line with the what we’re telling the user we’re doing, and makes the log in line with the value the user actually provided.

Also, would prefer carving this out into separate function to minimize bloating an already long init sequence.

Finally, nit: using `DEFAULT_DB_CACHE` instead of `DEFAULT_KERNEL_CACHE` seems more appropriate, even if they currently have the same
...

💬 stickies-v commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347370905)
It's a bit longer, but I find using a commonly used std lib function over a one-off macro more readable and more safe, but no big deal either way.

💬 jesterhodl commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3289763460)
> An internal name.

How about `uacomments` in `Total length of network version string () exceeds maximum length (). Reduce the number or size of uacomments.`
_msg1028

Similar to WalletDescriptor

💬 ryanofsky commented on issue "Assertion hit on shutdown of bitcoin-node with connected mining interface client":
(https://github.com/bitcoin/bitcoin/issues/33387#issuecomment-3289801227)
Nice find, I think this probably is a real bug. I wouldn't expect it to happen normally because there is an `Ipc::disconnectIncoming()` call in `Shutdown()` to close all IPC connections before the chainman object is freed. However, the `disconnectIncoming()` implementation is only closing the IPC connections without waiting for the objects and threads associated with them to be freed. So if an IPC client makes a request and the node receives it, and the node starts to shut down before the reques
...

💬 151henry151 commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3289823893)
## v30.0rc1 Testing

Tested on Debian 12 (bookworm) with kernel 6.1.0-37-amd64, 4-core x86_64 system. Ran through all the features from the testing guide:

- Datacarrier size: Created a transaction with 83 bytes of data in an OP_RETURN output and successfully sent it
- bitcoin command: Used `bitcoin node` to start a node and `bitcoin rpc` to make RPC calls - unified interface works well
- Bumpfee: Created a transaction and successfully used `bumpfee` with `replaceable: false`
- TRUC: Created rep
...

🤔 l0rinc reviewed a pull request: "coins: warn on oversized `-dbcache`"
(https://github.com/bitcoin/bitcoin/pull/33333#pullrequestreview-3222272959)
Thanks a lot for the comments and reproducers, added a @hodlinator and @stickies-v as coauthors.

I have split the PR into 3 commits, explained the descisions in the messages (cc: @willcl-ark), added unit tests for whatever I could extract, extracted the warning message to a method and exposed the warning for RPC (thanks @stickies-v).

💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347458082)
If any of you feel strongly, I can change it, but it was a deliberate choice from my part.
See https://godbolt.org/z/o3sKqefT5 for an ugly error on Windows that also convinced me to just use the shorter version. I could of course hack it with:
```C++
#ifdef _WIN32
#include <windows.h>
#undef max
#else
```
or just hope that https://github.com/bitcoin/bitcoin/blob/9f744fffc39d93c9966dec1d61f113a7521983ad/CMakeLists.txt#L261 fixes it, but `SIZE_MAX` seemed simpler and shorter in this case :/

-
...

💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347560408)
Added this in a separate commit (I'm not on my phone anymore :p), reviewers can check the commit message for instructions for how to reproduce it

💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347457548)
> using kernel_cache_sizes.coins instead of just inspecting -dbcache

* dbcache isn't always set (we'd duplicate work by setting the default explicitly). But we already have a duplicate strategy for very small memory environments, so this should be fine.
* `dbcache` is split to other indexes, this is the final value that is actually being used for dbcache. But you could argue that for total memory that's irrelevant.

> prefer carving this out into separate function

Agree

> using DEFAULT_DB_CAC
...

💬 l0rinc commented on issue "bench: unrealistic ConnectBlock benchmarks":
(https://github.com/bitcoin/bitcoin/issues/33375#issuecomment-3290237829)
@Raimo33 if you can provide an alternative that's reasonably simple for a micro-benchmark, I will happily review it.

But note that I already have a few micro-benchmark improvements in that area where review is needed:
* https://github.com/bitcoin/bitcoin/pull/32554 - creates a configurable block so that we can measure the composition of the block
* https://github.com/bitcoin/bitcoin/pull/32729/files#diff-547fa26a77a99ccd77aca7ce1c69c0544666f788d463dc7bff664001f9ff1c88R40 - splits checkblock mea
...

💬 l0rinc commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3290241610)
+1 for https://github.com/davidgumberg/bitcoin/commit/eda422cfa56245a9b78b9a4f499a490d44e58eb5 but without the explicit stuff, it's just noise, let the compiler do the work. We can add tests or static assertions instead of making simple structures more complicated - there's a reason the compiler is willing to generate it for us.

💬 l0rinc commented on pull request "p2p: Correct unrealistic headerssync unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3290253231)
reACK cc5dda1de333cf7aa10e2237ee2c9221f705dbd9

The only difference after the rebase is a removal of a `constexpr` in one of the `base_uint` constructors.

> changed PR-title (category) from "headerssync:" to "p2p

nit: the commits still state `headerssync` (fine by me, just noticed)

💬 hMsats commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3290439495)
Not sure if this is useful but anyway: Compiled bitcoin-30.0rc1 (it's called v30.0.0rc1 in debug.log) with cmake on Ubuntu 24.04.3 LTS on an Acer Aspire E1-572, 64 bits laptop with a 4 TB external ssd. All tests (test_bitcoin, test_bitcoin-qt) passed for both bitcoind and bitcoin-qt and bitcoin-qt looks fine. Upgraded bitcoind from version 29.0 to 30.0rc1. Bitcoind (30.0rc1) is working great (txindex=1) with low block verification times (about 1 second for "Saw new cmpctblock header" to "Update
...

💬 w0xlt commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3290444826)
Approach ACK.

> beneficial to extend this PR to show the reason for why script verification was enabled.

I agree.

> split the above suggestion into small focused commits

Commits could be squashed but but keeping them separate makes it clearer to review.

💬 w0xlt commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347899862)
Just to make clearer what `60 * 60 * 24 * 7 * 2` means.

```cpp
constexpr int64_t TWO_WEEKS = 14 * 24 * 60 * 60;
if (GetBlockProofEquivalentTime(... ) <= TWO_WEEKS) { ... }
```

Or (not tested)

```cpp
#include <chrono>
using namespace std::chrono;

constexpr auto TWO_WEEKS = 14d; // 14 days

if (GetBlockProofEquivalentTime(...) <= duration_cast<seconds>(TWO_WEEKS).count()) {
...
}
```

💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347916838)
Seems a bit risky this close to the release, maybe we can do that in a follow-up

💬 w0xlt commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347918277)
Provide safe fallback in release builds. Aborting solely because of logging seems unnecessary.

```suggestion
default: return "unknown reason";
```

💬 w0xlt commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347923281)
nit:
```suggestion
case CHECKED_HASH_NOT_IN_HEADERS: return "assumevalid hash not found in headers";
```

💬 w0xlt commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347925235)
nit: logs shouldn’t expose internal variable names

```suggestion
case CHECKED_BELOW_MIN_CHAINWORK: return "best header chainwork below -minimumchainwork";
```

🤔 w0xlt reviewed a pull request: "log: always print initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3222992176)
ACK https://github.com/bitcoin/bitcoin/pull/33336/commits/63dc937eea297332ec85c1abbcf3b94b8f74aa85 with non-blocking nits mentioned above.