💬 epiccurious commented on pull request "mempool: Loading progress added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932225974)
> logging the detailed progress in % is a bit overkill

+1 to @theStack's comment. Logging the start and end would be sufficient, no need for the 10% increments.

🚀 fanquake merged a pull request: "test: Fix CPartialMerkleTree.nTransactions signedness"
(https://github.com/bitcoin/bitcoin/pull/29363)

🤔 furszy reviewed a pull request: "wallet: track mempool conflicts with wallet transactions"
(https://github.com/bitcoin/bitcoin/pull/27307#pullrequestreview-1867972899)
light review, left few comments, nothing big.

💬 furszy commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481573074)
In 86aaf81f8:

If you touch this again; could pull `mit->second` into its own variable to improve readability.

```c++
const auto& wtx = mit->second;
// An output is considered spent by a spending transaction,
// unless the spending transaction is conflicted or abandoned.
if (wtx.isConfirmed() || wtx.InMempool() || (wtx.isInactive() && !wtx.isAbandoned()))
return true;
```

💬 furszy commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481616345)
Could document why it has been replaced here. It is not immediately evident only by reading the code.

💬 furszy commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481627720)
Would be good to document why you are doing this. I guess that you are cleaning up the mempool and wallet for the next test?

💬 furszy commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481600177)
Would be good to explain that you are doing the `mapWallet` lookup because the wtx could have been updated. Because otherwise, this could just be a `if (_it->second == conflict_txid) continue;` right?.

Also, as you already know that the output belongs to one of the wallet's txs, could:
```c++
CWalletTx& wtx = mapWallet.at(_it->second);
if (wtx.tx->GetWitnessHash() == conflict_wtxid) continue;
```

💬 furszy commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481609541)
Can remove the initial `wtx.mempool_conflicts.empty()` call and directly call `erase`.

```c++
auto try_updating_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) {
// Remove the previously conflicting transaction from this wtx's entry
wtx.mempool_conflicts.erase(conflict_txid);
if (wtx.mempool_conflicts.empty()) {
// If this wtx has no other mempool conflicts, it is now considered inactive
if (wtx.isMempoolConflicted()) {
wtx.m_stat
...

💬 furszy commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1481621542)
Would be good to document the behavior inside the code. It is not immediately evident only by reading the code.

💬 kevkevinpal commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1932261641)
force pushed [33153b0](https://github.com/bitcoin/bitcoin/pull/29402/commits/33153b0236346809c75f6f52a3000a4e18e3b385)

This removes the 10% increments and now logs a single line with the amount of mempool transactions being written to the disk

💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1932273223)
Green CI. Ready to go.

💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#issuecomment-1932273984)
Green CI. Ready to go.

💬 theuni commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1932303445)
@sipa Thanks.

@maflcko If we should decide to list the used identifiers for each `bitcoin-config.h` include as you've suggested here (which I'm not sure is necessary, but not opposed to either), we'd need to make sure to include the file where needed in each instance.

So I believe something like the above branch would be a requirement. And since @sipa and @hebasto agree, I think it makes sense to go ahead and do that either way.

I'll open a PR for that branch with instructions to reprod
...

💬 maflcko commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1481674077)
nit: Could add the mempool size in MB, if it is easy to add?

👍 maflcko approved a pull request: "mempool: Log added for dumping mempool transactions to disk"
(https://github.com/bitcoin/bitcoin/pull/29402#pullrequestreview-1868128189)
lgtm

💬 maflcko commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1481672254)
```suggestion
uint64_t mempool_transactions_to_write(vinfo.size());
```

nit: No need to repeat the type

💬 maflcko commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1481675864)
unrelated: In this file, I think it would be clearer to replace "disk" with "file", because the transactions are not dumped to separate files on disk, but to a single file.

💬 epiccurious commented on pull request "test: Fix utxo set hash serialisation signedness":
(https://github.com/bitcoin/bitcoin/pull/29399#issuecomment-1932348811)
Tested ACK fa0ceae970242d8d6bdef150c98f04c67b06e20c.

💬 kevkevinpal commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1481713668)
updated in [c23f5be](https://github.com/bitcoin/bitcoin/pull/29402/commits/c23f5beba2fbcea995b5dc7cab7c88fe0a2ecd22)
I just used `sizeof()` on `vinfo` to get this amount in bytes then multiplied by 1000000 to get MB

💬 kevkevinpal commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1481714088)
fix in [c23f5be](https://github.com/bitcoin/bitcoin/pull/29402/commits/c23f5beba2fbcea995b5dc7cab7c88fe0a2ecd22)