💬 jonatack commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2054492337)
Might be good to add a comment for this magic value, i.e. (from the PR description):

`Capped at 256 MiB, as gains are barely measurable for bigger batches (see PR 31645)`

also, clang-format

```diff
- /*hi=*/256_MiB
- );
+ /*hi=*/256_MiB);
```

and unneeded braces line 18

```diff
- (dbcache_bytes / DEFAULT_KERNEL_CACHE) * DEFAULT_DB_CACHE_BATCH,
+ dbcache_bytes / DEFAULT_KERNEL_CACHE * DEFAULT_DB_CACHE_BATCH,
```

💬 jonatack commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2054497017)
Feel free to ignore, but the following would be more readable and follow the most frequent convention in this codebase:

```diff
- if (const auto value = args.GetIntArg("-dbbatchsize")) options.batch_write_bytes = *value;
- else options.batch_write_bytes = GetDbBatchSize(args.GetIntArg("-dbcache", DEFAULT_KERNEL_CACHE));
+ if (const auto value = args.GetIntArg("-dbbatchsize")) {
+ options.batch_write_bytes = *value;
+ } else {
+ options.batch_write_bytes = GetDb
...

💬 jonatack commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2054511132)
lgtm (thanks for updating)

```
$ ./build/bin/bitcoind -help-debug | grep -A2 dbbatchsize
-dbbatchsize
Maximum database write batch size in bytes (default: calculated from the
`-dbcache` value or 16777216 if not set)
```

💬 achow101 commented on pull request "test: Test that migration automatically repairs corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2054516517)
Done

💬 achow101 commented on pull request "test: Test that migration automatically repairs corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2054516784)
Oops yes, fixed.

💬 achow101 commented on pull request "test: Test that migration automatically repairs corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2054517349)
Done

💬 maflcko commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2054552472)
std::in → stdin

💬 jonatack commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2822019017)
ACK cc8f239663a0874c307efa30815b40d9ef40badb

👋 maflcko's pull request is ready for review: "refactor: Avoid copies by using const references or by move-construction"
(https://github.com/bitcoin/bitcoin/pull/31650)

💬 maflcko commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2054580426)
Ok, restored initial version of this pull request. Should be easy to re-review.

💬 achow101 commented on pull request "docs: remove passing CI section in guidelines for PR merging as it's common sense":
(https://github.com/bitcoin/bitcoin/pull/32006#issuecomment-2822066744)
NACK, agree with @pinheadmz

👍 ryanofsky approved a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2784838614)
Code review ACK ece168ecef6e57e3a994ec5f7bd341f9c7525721. Changes since last review were squashing and rebasing, doing a better job of passing seed, timeouts, and tmpdirs to child processes. Previously dropped workaround for None errno is also back, this time with link to upstream python bug.

💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2054592532)
re: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2053031058

I think you could drop all unnecessary encoding and decoding by just dropping `encoding="utf-8"` parameter to `subprocess.run` and printing the bytes directly with something like `sys.stderr.buffer.write(b"<CHILD OUTPUT BEGIN>\n%s\n<CHILD OUTPUT END>" % e.output)`

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-2822100971)
Rebased e219f15976f50e0a703bc696b6445feb9950e65d -> eec7c49cf777cdcc53aeb20e31fbd5881c2b008b ([chainman_flush_destructor_4](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_4) -> [chainman_flush_destructor_5](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainman_flush_destructor_4..chainman_flush_destructor_5))

* Rebased to get a CI fix for windows cross

✅ maflcko closed a pull request: "docs: remove passing CI section in guidelines for PR merging as it's common sense"
(https://github.com/bitcoin/bitcoin/pull/32006)

💬 maflcko commented on pull request "docs: remove passing CI section in guidelines for PR merging as it's common sense":
(https://github.com/bitcoin/bitcoin/pull/32006#issuecomment-2822101827)
Closing for now, due to controversy.

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2053974854)
In "txgraph: Generalize GetClusterRefs to support subsections (preparation)" 542df1db60af9c2bb27f2b357a8b00817a4bfea0

Maybe check to ensure that `start_pos` is not more than `m_linearization.size()` to prevent out of range access attempt?

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2053878045)
In "txgraph: Add GetMainStagingDiagrams function (feature)" 3fffb59368b3a84973c6d775c349008273d72b91

I've read this comment several times, but I still don't fully understand it.

"or been in a cluster together with modified transaction",

```c++
/** Which transactions have been modified in the graph since creation, either directly or by
* being in a cluster which includes modifications. Only relevant for the staging graph. */
SetType modified;
```
Doesn't this imply that the trans
...

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2054497741)
In "txgraph: Introduce BlockBuilder interface (feature)" b76a9b719a9b7ee98b3835aab60369f37f1628a2

This is a bit hard to follow.


💬 maflcko commented on issue "ci: failure in interface_usdt_coinselection.py":
(https://github.com/bitcoin/bitcoin/issues/32322#issuecomment-2822121445)
Passing one: https://github.com/bitcoin/bitcoin/actions/runs/14580801695/job/40919798022#step:6:2537

`System info: Linux 6.8.0-1021-azure`

Failing one: https://github.com/bitcoin/bitcoin/actions/runs/14580801695/job/40896887951#step:6:2538

`System info: Linux 6.11.0-1012-azure`