💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1666757944)
Should we ideally check both?

🤔 furszy reviewed a pull request: "wallet: use LogTrace for walletdb log messages at trace level"
(https://github.com/bitcoin/bitcoin/pull/30355#pullrequestreview-2160642658)
ACK 46819f5df6de2a860c3ec87d55527b617a3b128f

💬 maflcko commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1666789017)
The reason to use `std::move` in `ChainstateManager` was not to optimize for anything, but to get the tidy-error for use-after-move, because using the options un-flattened may later on lead to an assertion error or other runtime error.

💬 maflcko commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2210854266)
pm-lgtm

💬 fjahr commented on pull request "rpc: Use untranslated error strings in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/30395#issuecomment-2210856839)
utACK fa22edc1ef4d4456dcde883acae6f38af142d7bf

💬 furszy commented on pull request "Fix cases of calls to `FillPSBT` errantly returning `complete=true`":
(https://github.com/bitcoin/bitcoin/pull/30357#discussion_r1666805504)
> I made a few changes in another test branch and I wonder if they make sense to you: https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:walletprocesspsbt-no-finalize-fixups

The question here is whether walletprocesspsbt should update the existing PSBT 'outputs' information (similar to how it updates the transaction inputs) or should only append data to it as it is currently doing. In the current test form, the first entry of the PSBT 'outputs' field ends up with two BIP32
...

💬 andrewtoth commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2210899073)
> disabling unrelated validations

I don't think @sipa said to disable validations. "benchmark pre-assumevalid IBD" means testing IBD (or reindex) before the assumevalid block, which right now is block 824,000. By setting `-assumevalid=0` that means you disabled using assumevalid entirely, so you will do signature checks from genesis.

> limiting `-stopatheight=200000`

With this limit you should not see any benefit from this change. Using default `dbcache` settings the cache is not flushe
...

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1666830623)
Yes, ideally in a separate, new test.

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1666853094)
Good feedback. I've inverted the `if` condition (and branch). Hope it is clearer now?

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1666853871)
I'd say DB obfuscation is different from `*.dat` obfuscation, but I've made the messages a bit more similar. Thanks!

🤔 tdb3 reviewed a pull request: "[WIP] net: return result from addnode RPC"
(https://github.com/bitcoin/bitcoin/pull/30381#pullrequestreview-2160865613)
Concept ACK
There is definitely value in ensuring errors are provided to the user (e.g. no longer failing silently for unsuccessful `onetry`).

I could see there being value in returning the peer id, but think that might be better off for a follow up PR. This one could focus on fixing the problem of masking the error (and prevent discussion on a return object vs null from impacting what is otherwise a relatively straightforward bug fix).

If this PR remains scoped to returning the returnin
...

🤔 maflcko reviewed a pull request: "fees: Remove CLIENT_VERSION serialization"
(https://github.com/bitcoin/bitcoin/pull/29702#pullrequestreview-2160857219)
rebased + addressed review

💬 maflcko commented on pull request "fees: Remove CLIENT_VERSION serialization":
(https://github.com/bitcoin/bitcoin/pull/29702#discussion_r1666897220)
> `static` ? Move it to the top of the file?

Sure, done both.

> Could consider defining three values:

I think one value is enough for now. One can always add more values, if they are used in the code in the future.

💬 maflcko commented on pull request "fees: Remove CLIENT_VERSION serialization":
(https://github.com/bitcoin/bitcoin/pull/29702#discussion_r1666886799)
> Storing `nVersionThatWrote` seems somewhat useful for manually debugging a corrupt fee data file, and figuring out how the bug occurred.

I'd say the write-timestamp of the file and the corresponding `debug.log` section is more helpful, because the debug log also contains the `CLIENT_VERSION` serialized (as string) (possibly with commit_id), as well as any other stuff and config that happened.

Happy to keep the value and instead write the 4 byte truncation of the commit id instead, fallin
...

📝 sipa opened a pull request: "random: add benchmarks and drop unnecessary Shuffle function"
(https://github.com/bitcoin/bitcoin/pull/30396)
This adds benchmarks for various operations on `FastRandomContext` and `InsecureRandomContext`, and then removes the ad-hoc `Shuffle` functions, now that it appears that standard library `std::shuffle` beats it. The other reason for keeping `Shuffle`, namely the fact that libstdc++ used self-move (which debug mode panics on) has been fixed as well (see https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1658344049).

💬 sipa commented on pull request "refactor: use existing RNG object in ProcessGetBlockData":
(https://github.com/bitcoin/bitcoin/pull/30393#issuecomment-2211020122)
It appears we can drop `Shuffle` entirely: https://github.com/bitcoin/bitcoin/pull/30396

💬 TheBlueMatt commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2211020658)
> I refuse to believe that re-implementing the entire networking stack, adding a noise protocol implementation and a new networking protocol is the best solution to having a non-RPC/non-poll based template provider.

I left a comment at https://github.com/stratum-mining/stratum/issues/1032#issuecomment-2211001835, but I don't buy that ZMQ is a reasonable way forward here. To be clear, the goal is not to *just* have "a non-RPC/non-poll based template provider", but rather to decentralize mining
...

📝 maflcko converted_to_draft a pull request: "refactor: use existing RNG object in ProcessGetBlockData"
(https://github.com/bitcoin/bitcoin/pull/30393)
Two small rand fixups. The debug mode one is a follow-up to commit 7af95afa8bc3f36a37d082ef3475cb3e0bd3a0e4 and https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1658344049. The RNG object one to commit 8e31cf9c9b5e9fdd01e8b220c08a3ccde5cf584c

👋 maflcko's pull request is ready for review: "refactor: use existing RNG object in ProcessGetBlockData"
(https://github.com/bitcoin/bitcoin/pull/30393)

💬 maflcko commented on pull request "refactor: use existing RNG object in ProcessGetBlockData":
(https://github.com/bitcoin/bitcoin/pull/30393#issuecomment-2211029344)
Nice. Dropped that commit here.