💬 hodlinator commented on pull request "net: Disconnect message follow-ups to #28521":
(https://github.com/bitcoin/bitcoin/pull/31633#discussion_r1997333029)
(The buggy reconnection attempts are handled in new PR #32073).
(https://github.com/bitcoin/bitcoin/pull/31633#discussion_r1997333029)
(The buggy reconnection attempts are handled in new PR #32073).
💬 hodlinator commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#issuecomment-2727012030)
Thanks for testing!
> I could not recreate a race condition but I have
Did you try the recommended approach in the commit message of 3301d2cbe8c3b76c97285d75fa59637cb6952d0b? It is a bit terse.
* Make sure you don't have the added `sync_txindex` fix on the Python side.
* Add the `sleep_for()` in the recommended place in the C++ side, (build).
* You should be able to get the individual tests to fail (simplest is to run the test_runner.py suite so the `--args` get sent in correctly).
...
(https://github.com/bitcoin/bitcoin/pull/32010#issuecomment-2727012030)
Thanks for testing!
> I could not recreate a race condition but I have
Did you try the recommended approach in the commit message of 3301d2cbe8c3b76c97285d75fa59637cb6952d0b? It is a bit terse.
* Make sure you don't have the added `sync_txindex` fix on the Python side.
* Add the `sleep_for()` in the recommended place in the C++ side, (build).
* You should be able to get the individual tests to fail (simplest is to run the test_runner.py suite so the `--args` get sent in correctly).
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2727013588)
Updated 21f6a3de77a9eedcca5d47f694d540d42b3ddbcc -> 5991a69ee0000de551955846d7d21733c326a748 ([kernelApi_28](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_28) -> [kernelApi_29](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_29), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_28..kernelApi_29))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1989793614), removed outdated comment about the validation interface i
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2727013588)
Updated 21f6a3de77a9eedcca5d47f694d540d42b3ddbcc -> 5991a69ee0000de551955846d7d21733c326a748 ([kernelApi_28](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_28) -> [kernelApi_29](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_29), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_28..kernelApi_29))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1989793614), removed outdated comment about the validation interface i
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1997340094)
Done.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1997340094)
Done.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1997340229)
I'm not quite sure what you meant with "Perhaps an alternative approach would be to keep the integer values between kernel_LogCategory the same as BCLog::LogFlags and just define a kernel-specific bitfield that defines which BCLog flags are valid". Does the current approach work for you?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1997340229)
I'm not quite sure what you meant with "Perhaps an alternative approach would be to keep the integer values between kernel_LogCategory the same as BCLog::LogFlags and just define a kernel-specific bitfield that defines which BCLog flags are valid". Does the current approach work for you?
💬 hodlinator commented on pull request "fuzz: More accurate coverage reports":
(https://github.com/bitcoin/bitcoin/pull/30156#discussion_r1997340468)
(Worked on in #32059).
(https://github.com/bitcoin/bitcoin/pull/30156#discussion_r1997340468)
(Worked on in #32059).
💬 aiswaryasankar commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997476939)
The function `AddI2PSessionsIfNeeded` is called without checking if I2P is enabled, potentially leading to unnecessary session creation.
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997476939)
The function `AddI2PSessionsIfNeeded` is called without checking if I2P is enabled, potentially leading to unnecessary session creation.
💬 aiswaryasankar commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997476940)
The `AddI2PSessionsIfNeeded` function is declared with `EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex)`, which is incorrect. This annotation suggests that the function requires the mutex to be unlocked, but it should require the mutex to be locked to ensure thread safety when modifying `m_unused_i2p_sessions`.
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997476940)
The `AddI2PSessionsIfNeeded` function is declared with `EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex)`, which is incorrect. This annotation suggests that the function requires the mutex to be unlocked, but it should require the mutex to be locked to ensure thread safety when modifying `m_unused_i2p_sessions`.
💬 aiswaryasankar commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997476976)
The function `AddI2PSessionsIfNeeded` is called without checking if I2P is enabled, which may lead to unnecessary session creation.
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997476976)
The function `AddI2PSessionsIfNeeded` is called without checking if I2P is enabled, which may lead to unnecessary session creation.
💬 aiswaryasankar commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997476977)
The `AddI2PSessionsIfNeeded` function is declared with `EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex)`, which is incorrect. The `!` operator is not valid in this context and should be removed to ensure proper lock requirements.
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997476977)
The `AddI2PSessionsIfNeeded` function is declared with `EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex)`, which is incorrect. The `!` operator is not valid in this context and should be removed to ensure proper lock requirements.
💬 aiswaryasankar commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997477829)
The function `AddI2PSessionsIfNeeded` is called without checking if I2P is enabled, potentially leading to unnecessary session creation.
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997477829)
The function `AddI2PSessionsIfNeeded` is called without checking if I2P is enabled, potentially leading to unnecessary session creation.
💬 aiswaryasankar commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997477830)
The `AddI2PSessionsIfNeeded` function is declared with `EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex)`, which indicates that the function should not be called while holding the `m_unused_i2p_sessions_mutex`. However, the function is called within `CConnman::OpenNetworkConnection`, which is marked with the same lock requirement, potentially leading to a deadlock if the lock is held elsewhere in the call stack.
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997477830)
The `AddI2PSessionsIfNeeded` function is declared with `EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex)`, which indicates that the function should not be called while holding the `m_unused_i2p_sessions_mutex`. However, the function is called within `CConnman::OpenNetworkConnection`, which is marked with the same lock requirement, potentially leading to a deadlock if the lock is held elsewhere in the call stack.
💬 aiswaryasankar commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997481576)
The function `AddI2PSessionsIfNeeded` is called without checking if `m_i2p_sam_session` is null, potentially leading to unnecessary session creation.
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997481576)
The function `AddI2PSessionsIfNeeded` is called without checking if `m_i2p_sam_session` is null, potentially leading to unnecessary session creation.
💬 aiswaryasankar commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997481577)
The `AddI2PSessionsIfNeeded` function is declared with `EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex)`, which is incorrect. The `!` operator is not valid in this context and should be removed to ensure proper locking behavior.
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997481577)
The `AddI2PSessionsIfNeeded` function is declared with `EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex)`, which is incorrect. The `!` operator is not valid in this context and should be removed to ensure proper locking behavior.
💬 aiswaryasankar commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997486622)
The function `AddI2PSessionsIfNeeded` is called without checking if I2P is enabled, potentially leading to unnecessary session creation.
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997486622)
The function `AddI2PSessionsIfNeeded` is called without checking if I2P is enabled, potentially leading to unnecessary session creation.
💬 aiswaryasankar commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997486623)
The `AddI2PSessionsIfNeeded` function is declared with `EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex)`, which is incorrect. It should require the lock, not the absence of it.
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997486623)
The `AddI2PSessionsIfNeeded` function is declared with `EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex)`, which is incorrect. It should require the lock, not the absence of it.
💬 aiswaryasankar commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997488140)
The function `AddI2PSessionsIfNeeded` is called without checking if `m_i2p_sam_session` is null, which can lead to unnecessary session creation and potential resource leaks.
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997488140)
The function `AddI2PSessionsIfNeeded` is called without checking if `m_i2p_sam_session` is null, which can lead to unnecessary session creation and potential resource leaks.
💬 aiswaryasankar commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997488147)
The `AddI2PSessionsIfNeeded` function is declared with `EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex)`, which is incorrect. The `!` negation implies that the lock should not be held, but the function likely needs the lock to safely modify `m_unused_i2p_sessions`.
(https://github.com/bitcoin/bitcoin/pull/32065#discussion_r1997488147)
The `AddI2PSessionsIfNeeded` function is declared with `EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex)`, which is incorrect. The `!` negation implies that the lock should not be held, but the function likely needs the lock to safely modify `m_unused_i2p_sessions`.
📝 ajtowns converted_to_draft a pull request: "ArgsManager: support subcommand-specific options"
(https://github.com/bitcoin/bitcoin/pull/28802)
Adds the ability to link particular options to one or more subcommands, and uses this feature in `bitcoin-wallet`. Separates out the help information for subcommand-specific options (duplicating it if an option applies to multiple subcommands), and provides a function for checking if some options have been specified that only apply to different subcommands.
(https://github.com/bitcoin/bitcoin/pull/28802)
Adds the ability to link particular options to one or more subcommands, and uses this feature in `bitcoin-wallet`. Separates out the help information for subcommand-specific options (duplicating it if an option applies to multiple subcommands), and provides a function for checking if some options have been specified that only apply to different subcommands.
💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-2727215147)
Leaving as draft pending #31250
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-2727215147)
Leaving as draft pending #31250