💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2195728404)
Addressed some of @instagibbs' comments, which involved moving some of the `src/test/fuzz/cluster_linearize.cpp` code to a common `src/test/util/cluster_linearize.h`, where it is also available to a newly-added (but pretty barebones) `src/test/cluster_linearize_tests.cpp`.

🚀 ryanofsky merged a pull request: "Mining interface followups, reduce cs_main locking, test rpc bug fix"
(https://github.com/bitcoin/bitcoin/pull/30335)

🤔 cbergqvist reviewed a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2146541283)
`git range-diff master a8f8a2d 9700c217`

Agree with @furszy on reordering the commits to make it possible to retain the 3 asserts in `MigrateToDescriptor()`.

💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1657873459)
This made me cringe when first looking at the code too, but I guess retaining the inconsistency makes it easier to search for `AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey)` and find all 4 places it exists. Changing it in this PR would arguably add noise (on the other hand - if not now, then when?).

💬 ajtowns commented on pull request "logging: Use LogFatal instead LogError for fatal errors":
(https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1657886163)
The medium post describes five levels with the same names as ours currently: error, warn, info, debug, trace; the other six links all describe syslog's behaviour, which was designed for unix systems in the 1980s. Logging systems we actually use include python's (which has 5 default levels), leveldb (which does not have levels, so we treat it all as debug), and libevent (which has 4 levels). So, no, 6-8 levels is not standard, and for a custom subsystem we should be doing what's best for us, not
...

📝 cobratbq opened a pull request: "doc: external-signer, example 'createwallet' RPC call requires eight argument"
(https://github.com/bitcoin/bitcoin/pull/30354)
The eight argument 'true' is necessary to invoke the external signer upon creating the wallet. The parameter defaults to 'false' otherwise.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658060325)
> With that, this PR could add only DIRTY coins and ignore the FRESH flag, which would be simpler.

I'm not sure how that would be simpler. The two flags are coupled and so adding one and not the other would be a more complex diff. Perhaps I don't understand what you are suggesting?

I could just replace the single FRESH-but-not-DIRTY check with an assertion and remove this comment about tracking FRESH coins?

💬 maflcko commented on pull request "doc: external-signer, example 'createwallet' RPC call requires eight argument":
(https://github.com/bitcoin/bitcoin/pull/30354#discussion_r1658229564)
Could use named arguments?

🤔 alfonsoromanz reviewed a pull request: "test: Added coverage to Block not found error using gettxoutsetinfo"
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2147191277)
Re ACK 82c454c65658439481b50fe71ed097ecb8d70737

💬 maflcko commented on pull request "Mining interface followups, reduce cs_main locking, test rpc bug fix":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1658270912)
nit: The same can be done below where `tip` is assigned again.

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2196327312)
ACK 15cc11b4e591f4c67fcdefb224fadf4383064340 🙂

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 15cc11b4e591f4c67fcdefb224
...

📝 ajtowns opened a pull request: "wallet: use LogTrace for walletdb log messages at trace level"
(https://github.com/bitcoin/bitcoin/pull/30355)
Wallet sqlite logging is enabled by `-debug=walletdb -loglevel=walletdb:trace` however the actual log messages are sent at `BCLog::Level::Info`. Switch to the trace level to make this consistent. This adds `[walletdb:trace]` to the log output, eg:

```
[httpworker.3] [wallet/sqlite.cpp:55] [TraceSqlCallback] [/tmp/bitcoin_func_test_4fsnatpg/node0/regtest/wallets/boring/wallet.dat] SQLite Statement: BEGIN EXCLUSIVE TRANSACTION
```

becomes

```
[httpworker.0] [wallet/sqlite.cpp:55] [Trac
...

👍 rkrux approved a pull request: "test: fix inconsistency in fundrawtransaction weight limits test"
(https://github.com/bitcoin/bitcoin/pull/30353#pullrequestreview-2147327181)
tACK [4f571fb](https://github.com/bitcoin/bitcoin/pull/30353/commits/4f571fb73526b55a83af3c04fbc662ccdd1307ae)

`make, test/functional` are successful.

Verified that the default value of `add_inputs` is `True` in `fundrawtransaction` that might have caused the inconsistency in the CI: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/spend.cpp#L751

Also verified that the corresponding scenario in `wallet_send.py` has `add_inputs: False` passed already here: https://github.com
...

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1658344049)
unrelated nit: The debug mode part has been fixed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85828#c3 / https://github.com/gcc-mirror/gcc/commit/c2fb0a1a2e7a0fb15cf3cf876f621902ccd273f0 for all version supported by Bitcoin Core, so this part can be removed.

About whether the stdlib is faster or uses more entropy, I haven't benchmarked it, but wanted to note that it generates two ints with one 64-bit rng call and uses https://github.com/lemire/FastShuffleExperiments

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1658346921)
nit: Could also require `concept std::uniform_random_bit_generator` inside of `RandomNumberGenerator`?

📝 Sjors opened a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356)
When generating a block template through e.g. getblocktemplate RPC, we reserve 4000 weight units and 400 sigops. Pools use this space for their coinbase outputs.

At least one pool patched their Bitcoin Core node to adjust these hardcoded values. They eventually [produced an invalid block](https://bitcoin.stackexchange.com/questions/117837/how-many-sigops-are-in-the-invalid-block-783426) which exceeded the sigops limit.

The existince of such patches suggests it may be useful to make this va
...

💬 Sjors commented on pull request "Mining interface followups, reduce cs_main locking, test rpc bug fix":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1658383394)
Added to #30356.

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2196451678)
I opened #30356 with some `BlockAssembler::Options` and mining interface refactoring that should make this PR smaller.

💬 maflcko commented on pull request "wallet: use LogTrace for walletdb log messages at trace level":
(https://github.com/bitcoin/bitcoin/pull/30355#issuecomment-2196451807)
ACK 46819f5df6de2a860c3ec87d55527b617a3b128f

Makes sense to use the severity level and category that the user asked for, instead of a different one.

Could also change it to `[warn]` when the user request could not be fulfilled?

https://github.com/bitcoin/bitcoin/blob/46819f5df6de2a860c3ec87d55527b617a3b128f/src/wallet/sqlite.cpp#L270

👍 maflcko approved a pull request: "Several randomness improvements"
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2147384765)
Left some more nits. Feel free to ignore.