💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2226843029)
@andrewtoth, I'm running the benchmark now on a cosy Hetzner dedicated server (without a local Bitcoin node, let's see if that makes it too volatile).

Does this seem like a realistic measurement?
```bash
hyperfine \
--warmup 1 --runs 2 \
--show-output \
--export-markdown bench.md \
--parameter-list COMMIT 0383de,21090d \
--prepare 'git checkout {COMMIT} && make -j10 && rm -rf /mnt/hdd1/BitcoinData/*' \
'echo {COMMIT} && ./src/bitcoind -datadir=/mnt/hdd1/BitcoinData -dbcache=16384 -pru
...

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2226847354)
I wrote:

> I'll expand the demo app to simulate all the steps a Template Provider would do

Once that works the next step for me would be to open an alternative to #29432 which runs the Template Provider in `bitcoin-tp`. I might wait with that until we ship a release with this interface enabled on the node side, ideally in `bitcoind` and `bitcoin-qt` rather than in a seperate `bitcoin-node` and `bitcoin-gui` binaries.

I could then modify the guix build to _only_ produce `bitcoin-tp`. Thi
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676801669)
this seems redundant, if we're not sure about this, I'd rather add a test that can bite back (as you can tell, I'm not a big fan of dead comments :p)

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676800505)
nit: "move coin into us" sounds a bit weird.
And now that the code is cleaner, does the comment add anything that the code doesn't already say?

* `Since this entry will be erased` -> `if (cursor.WillErase(*it)) {`
* `we can move the coin into us instead of copying it` -> `std::move(it->second.coin)` vs `it->second.coin`

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676801797)
+1

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676802135)
this should have been there all along, i.e. you've fixed a small independent accounting bug here, right?

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676801434)
I believe in making important parts of the code stick out and secondary importance going in the background.
This move is secondary importance, right? But it occupies a big part of the method, maybe we can simplify it with a ternary:
```suggestion
CCoinsCacheEntry& entry{itUs->second};
entry.coin = cursor.WillErase(*it) ? std::move(it->second.coin) : it->second.coin;
```

📝 TheCharlatan opened a pull request: "fuzz: Deglobalize signature cache in sigcache test"
(https://github.com/bitcoin/bitcoin/pull/30447)
The body of the fuzz test should ideally be a pure function. If data is persisted in the cache over many iterations, and there is a crash, reproducing it from the input might be difficult. Solve this by getting rid of the global state. This is a follow-up from #30425.

💬 0xB10C commented on pull request "fix: rendering an amp characters in the wallet name for QMenu":
(https://github.com/bitcoin/bitcoin/pull/30446#issuecomment-2226870406)
Thanks for the contribution knst. I think this PR might be better suited for https://github.com/bitcoin-core/gui. The gui repo is regularly synced with the bitcoin/bitcoin repo.

💬 hebasto commented on pull request "fix: rendering an amp characters in the wallet name for QMenu":
(https://github.com/bitcoin/bitcoin/pull/30446#issuecomment-2226873163)
> Thanks for the contribution knst. I think this PR might be better suited for https://github.com/bitcoin-core/gui. The gui repo is regularly synced with the bitcoin/bitcoin repo.

Yes. @knst please close this PR and open it in https://github.com/bitcoin-core/gui.

👍 hodlinator approved a pull request: "rest: Reject negative outpoint index early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30444#pullrequestreview-2176241736)
ACK fab54db9f1d0e634f4a697480dbb87b87940dc5c

Good to see more improvements to the robustness of that part of **rest.cpp**.
Ran the test-change without the C++ modifications and verified that signed (+/-) output indexes were previously supported. As that capability served no purpose and is unlikely to have been used, I support this narrowing of the API.

Also noticed what appears to be missing test coverage of `Parse[U]Int*`:

```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_
...

📝 knst opened a pull request: "fix: rendering an amp characters in the wallet name for QMenu"
(https://github.com/bitcoin-core/gui/pull/828)
In the current implementation Qt uses '&' as a signal to underscore letter and use it as a hot-key, which is not expected for case of wallet name.

The [comment in the code](https://github.com/bitcoin/bitcoin/pull/30446/files#diff-2ecf8cbf369cf3d2f3d2b1cf5cfe4c1a647d63e11e2885d2fd0ac11fb5f7a804L402-L404) is misleading, if you replace one & to double &&, the next & will be a "single one". All of them in the string should be replaced, not only the first occurrence.


See screenshots before &
...

✅ knst closed a pull request: "fix: rendering an amp characters in the wallet name for QMenu"
(https://github.com/bitcoin/bitcoin/pull/30446)

💬 knst commented on pull request "fix: rendering an amp characters in the wallet name for QMenu":
(https://github.com/bitcoin/bitcoin/pull/30446#issuecomment-2226884796)
closed as moved to bitcoin-core/gui: https://github.com/bitcoin-core/gui/pull/828

👍 hodlinator approved a pull request: "rpc: Use CHECK_NONFATAL over Assert"
(https://github.com/bitcoin/bitcoin/pull/30429#pullrequestreview-2176246261)
ACK fa6270737eb9655bfb4e29b7070ecb6cd2087b7f

Ran linter without the C++ change:
```console
$ ./test/lint/lint-assertions.py
CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.
src/rpc/blockchain.cpp:804: const auto& first_unpruned{*Assert(blockman.GetFirstBlock(*chain_tip, /*status_mask=*/BLOCK_HAVE_MASK, first_block))};
src/rpc/blockchain.cpp:811: return Assert(first_unpruned.pprev)->nHeight;
```
Including the C++ change silences the
...

👍 hodlinator approved a pull request: "refactor: Remove Span operator==, Use std::ranges::equal"
(https://github.com/bitcoin/bitcoin/pull/29071#pullrequestreview-2176250245)
ACK fabcd0c4fe44e3bc2d6a59f2839f459fd5c81171

💬 hodlinator commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1676823487)
nit: Double newline

💬 hodlinator commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1676825369)
(Attempted switching around the parameters to `lexicographical_compare()` and confirmed that it broke `test_bitcoin -t db_tests`).

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2226936005)
> @andrewtoth, I'm running the benchmark now on a cosy Hetzner dedicated server (without a local Bitcoin node, let's see if that makes it too unreliable).
>
> Does this seem like a realistic measurement?
>
> ```shell
> hyperfine \
> --warmup 1 --runs 2 \
> --show-output \
> --export-markdown bench.md \
> --parameter-list COMMIT 0383de,21090d \
> --prepare 'git checkout {COMMIT} && make -j10 && rm -rf /mnt/hdd1/BitcoinData/*' \
> 'echo {COMMIT} && ./src/bitcoind -datadir=/mnt/hdd1/Bi
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1676837143)
Yes!