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

💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1481748419)
The option's help text states `Set SQLite synchronous=OFF to disable waiting for the database to sync to disk. This is unsafe and can cause data loss and corruption. This option is only used by tests to improve their performance (default: false)`

💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1480545212)
Doesn't this create an unnecessary indirection?

`NetPermissions::Initialize` will call `NetPermissions::ClearFlag`, clear the `Implicit` flag, and call `NetPermissions::Initialize` again, which will simply return given the flag is not set anymore

💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1480554336)
Just curious, is there a reason to initialize this if you are overwriting it during `Options::Init`? (i.e. is this ever being used without initializing?)

🤔 sr-gi reviewed a pull request: "p2p: Allow whitelisting manual connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1866296281)
Approach ACK [1d0216e](https://github.com/bitcoin/bitcoin/pull/27114/commits/1d0216ed3225056871fee8291e43f24e011d9bdf)

💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1480525652)
Aren't these flags also used for manuals now?