💬 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.

💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347935159)
This was deliberate, are you suggesting that's not grammatically correct?
"File not found" -> we don't usually say "File *is* not found". The internets tell me this is called "elliptical sentence" with "implied copula".

💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347936278)
I'm curious what others think. The current one makes the error searchable, that's why I left it as such.