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

💬 fjahr commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#issuecomment-2211146256)
Rebased and addressed comments

> Nit: IMO, https://github.com/bitcoin/bitcoin/commit/64d35be20c43f764e5c22657085a790d0adcfe22 message should be amended to also include nTxCount and nTxDiff, since it's not just nChainTx that's changing to uint64_t.

Naming all the local variables that are touched isn't necessary I think but I added a comment in the commit description.

👍 stickies-v approved a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2161063572)
re-ACK 606a7ab862470413ced400aa68a94fd37c8ad3d3

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1667015153)
nit: having `DEFAULT_VALIDATION_CACHE_BYTES` and `DEFAULT_SCRIPT_EXECUTION_CACHE_BYTES` defined in `sigcache.h` is not ideal. One alternative is to rename `script/sigcache.h` to `script/cache.h` and move `ValidationCache` in it, but that touches quite a few lines so I'm not convinced that's the best thing to do for this PR to make progress.

👍 dergoegge approved a pull request: "psbt: Check non witness utxo outpoint early"
(https://github.com/bitcoin/bitcoin/pull/29855#pullrequestreview-2161086689)
utACK 9e13ccc50eec9d2efe0f472e6d50dc822df70d84

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2211245443)
> We've seen time and time again "just run this sidecar software next to Bitcoin Core" is not a thing that anyone does.

Yet another approach for said sidecar is to use IPC. It offers first class access to the node, e.g. via the newly added mining interface. It can probably automagically find a running Bitcoin Core process or spin one up. This would require more (review) progress in #10102 and additional tooling so external application can easily use IPC.

🤔 BrandonOdiwuor reviewed a pull request: "Fix cases of calls to `FillPSBT` errantly returning `complete=true`"
(https://github.com/bitcoin/bitcoin/pull/30357#pullrequestreview-2161119839)
Concept ACK

💬 fjahr commented on pull request "rpc: Use untranslated error strings in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/30395#issuecomment-2211301127)
re-ACK fa5b8920be041380fbfa4c7b443918637423d7a0