💬 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)`
(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
(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?)
(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)
(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?
(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
(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?
(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
(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
(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`?
(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"
(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
...
(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
(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
💬 TheCharlatan commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#discussion_r1481702805)
Nit: No need for these formatting-only changes?
(https://github.com/bitcoin/bitcoin/pull/29114#discussion_r1481702805)
Nit: No need for these formatting-only changes?
👍 TheCharlatan approved a pull request: "util: Faster std::byte (pre)vector (un)serialize"
(https://github.com/bitcoin/bitcoin/pull/29114#pullrequestreview-1868175505)
ACK fab41697a5448ef2861f65795bd63a4ccdda6a40
It's a nice cleanup, but I don't observe a big speedup on my machine after applying the diff and running the bench mentioned in the description on top of fab41697a5448ef2861f65795bd63a4ccdda6a40.
(https://github.com/bitcoin/bitcoin/pull/29114#pullrequestreview-1868175505)
ACK fab41697a5448ef2861f65795bd63a4ccdda6a40
It's a nice cleanup, but I don't observe a big speedup on my machine after applying the diff and running the bench mentioned in the description on top of fab41697a5448ef2861f65795bd63a4ccdda6a40.
💬 sdaftuar commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1932465965)
Using a datalogging and replaying system that I have, I did some analysis of what the effect the idea of imbuing LN commitment transaction spends with v3 semantics would have had in 2023. My writeup of that is here: https://delvingbitcoin.org/t/analysis-of-attempting-to-imbue-ln-commitment-transaction-spends-with-v3-semantics/527
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1932465965)
Using a datalogging and replaying system that I have, I did some analysis of what the effect the idea of imbuing LN commitment transaction spends with v3 semantics would have had in 2023. My writeup of that is here: https://delvingbitcoin.org/t/analysis-of-attempting-to-imbue-ln-commitment-transaction-spends-with-v3-semantics/527
💬 hebasto commented on pull request "indicate explicit to the user that the wallet balances shown is watch only.":
(https://github.com/bitcoin-core/gui/pull/37#issuecomment-1932474790)
Closing due to lack of support from the author.
(https://github.com/bitcoin-core/gui/pull/37#issuecomment-1932474790)
Closing due to lack of support from the author.
✅ hebasto closed a pull request: "indicate explicit to the user that the wallet balances shown is watch only."
(https://github.com/bitcoin-core/gui/pull/37)
(https://github.com/bitcoin-core/gui/pull/37)
💬 furszy commented on pull request "init: settings, do not load auto-generated warning msg":
(https://github.com/bitcoin/bitcoin/pull/29301#discussion_r1481825039)
Sure, sounds good @ryanofsky. Thanks for coming back to this!
(https://github.com/bitcoin/bitcoin/pull/29301#discussion_r1481825039)
Sure, sounds good @ryanofsky. Thanks for coming back to this!
💬 hebasto commented on pull request "debugwindow: update session ID tooltip":
(https://github.com/bitcoin-core/gui/pull/788#issuecomment-1932536186)
> When you have a v2 connection, there is always a session ID.
What if the connection type is still `DETECTING`?
The code reference:https://github.com/bitcoin-core/gui/blob/6737331c4ccc6da578bb44c809cc4e18f017ba51/src/node/connection_types.h#L83-L88
(https://github.com/bitcoin-core/gui/pull/788#issuecomment-1932536186)
> When you have a v2 connection, there is always a session ID.
What if the connection type is still `DETECTING`?
The code reference:https://github.com/bitcoin-core/gui/blob/6737331c4ccc6da578bb44c809cc4e18f017ba51/src/node/connection_types.h#L83-L88