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

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1658360003)
nit: This is used in 5 places, in 3 of them "incorrectly". (BIP 330 doesn't disallow a uint64_t max as salt, same for `CSipHasher` in `PriorityComputer`). So you could replace them with a call to `rand`. The remaining two could just be inlined with a call to randrange.

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1658362617)
nit in b128d1d0ab48d2c4e535ff0e90225d6aae626817: When making `GetOSRand` private, you should make this one private as well?

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1658389327)
I benchmarked this suggested change and it was an improvement for my CPU.

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2196470058)
The MSan, depends job [failed](https://cirrus-ci.com/task/5872614726434816?logs=ci#L2697) the test which issues the certificate 1 second in the future. I increased the time difference so such failure is less likely.

The fuzzer could also pick 1 second in the past or future for the certificate validity test. But it uses `SetMockTime`, so presumably doesn't have this problem.

💬 maflcko commented on pull request "Make GUI and CLI tools use the same datadir":
(https://github.com/bitcoin/bitcoin/pull/27409#issuecomment-2196478179)
@ryanofsky What is the status of this?

💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1658405291)
Good catch, this is indeed wrong.