💬 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

💬 maflcko commented on pull request "rpc: Use untranslated error strings in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/30395#discussion_r1666958843)
Thx, removed `.`.

💬 fjahr commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#discussion_r1666968422)
Done

💬 fjahr commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#discussion_r1666968543)
This would mean that we return a different key in the JSON returned from the RPC. We usually don't do this unless it's needed in order to not break things on the user side. We also only use the `m_` prefix for class members.

🤔 pablomartin4btc reviewed a pull request: "Feature: Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/pull/29838#pullrequestreview-2160987527)
Concept ACK and light tACK 50f4ca2363e008aacb9f8a8aa1bb7abba04c0b33

Tried different challenges and custom `-datadir` option.

👍 tdb3 approved a pull request: "rpc: Use untranslated error strings in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/30395#pullrequestreview-2160988753)
ACK fa5b8920be041380fbfa4c7b443918637423d7a0

Re-ran check in https://github.com/bitcoin/bitcoin/pull/30395#discussion_r1666952404 . No extra period seen.
Ran unit/functional tests locally, including `feature_assumeutxo` (all passed).