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

💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1480549987)
Why are we splitting the parsing of `NetPermissionFlags::Implicit` between here and `NetPermissions::Initialize`? I find this a bit weird

💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1481678841)
Any reason why this was moved from `Connman::CreateNodeFromAcceptedSocket` to here?

💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1480631259)
It may be worth documenting this in the method docs to prevent potential future footshots

💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1481727947)
nit: This comment is not relevant anymore. Either replace it with the one right before `self.noban_tx_relay = True` or remove it

💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1480633888)
Also, if kept, wouldn't it be better to set it to the default constant defined in `net_permission.h`?

💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1480624819)
What is the expected behavior if only the direction is specified? e.g. "in@1234"

💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1932412438)
> > To avoid this situation, we need to prevent the multiple batches from writing at the same time. Although SQLite does have facilities for doing that, they require the Write Ahead Log (WAL) mode, which is a feature that we explicitly chose to disable since it spread wallet data to multiple files. As such, we need to be handling the concurrency ourselves.
>
> This doesn't seem accurate to me. As long as all the batch objects are sharing a `sqlite3* m_db` pointer and using the same connection
...

💬 kevkevinpal commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1481757491)
should I update `disk` to `file` in this PR? I was trying to write the logs similar to the other ones in this file