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

💬 sipa commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2211033120)
I've also benchmarked Deniel Lemire's nearly [divisionless random integer generation](https://lemire.me/blog/2019/06/06/nearly-divisionless-random-integer-generation-on-various-systems/), which libstdc++'s `std::shuffle` switched to for `randrange` itself, but opted not to include this in this PR. For `InsecureRandomContext` it's unambiguously faster than the current code (but nothing performance-critical relies on `InsecureRandomContext::randrange`, so far). For `FastRandomContext` the advantag
...

💬 maflcko commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#discussion_r1666921146)
Are you sure this commit compiles?

💬 maflcko commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#discussion_r1666929149)
Could just squash commit 2+3?

💬 sipa commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#discussion_r1666929156)
I'm sure it does not. Fixed.

🤔 tdb3 reviewed a pull request: "rpc: Use untranslated error strings in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/30395#pullrequestreview-2160961016)
Approach ACK

💬 tdb3 commented on pull request "rpc: Use untranslated error strings in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/30395#discussion_r1666952404)
nit: It looks like there might be an extraneous period for at least some error messages (between the error message and the parenthesis-wrapped path).

Created a snapshot with `dumptxoutset`.
Modified a byte in the snapshot to induce failure during load.
```
$ src/bitcoin-cli loadtxoutset mytxoutset.dat
error code: -32603
error message:
Unable to load UTXO snapshot: assumeutxo block hash in snapshot metadata not recognized (hash: 767fccc851802c0180a3ebf1631ab978d4d6a5ae1d452dc26e9c1778507
...

💬 maflcko commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1666955956)
first commit: could adjust the test

```
src/test/amount_tests.cpp:86: // Maximum size in bytes, should not crash
```

To `std::numeric_limits<uint32_t>::max() >> 1` ?


And

```
src/test/fuzz/fee_rate.cpp:23: const auto bytes = fuzzed_data_provider.ConsumeIntegral<uint32_t>();
```

to `fuzzed_data_provider.ConsumeIntegral<uint64_t>() >> 1`?

🤔 levantah reviewed a pull request: "policy: Enable full-rbf by default"
(https://github.com/bitcoin/bitcoin/pull/28132#pullrequestreview-2160969308)
ACK fbdc61e