💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472955815)
In commit "wallet: track mempool conflicts" (a6ae5b23b1497ab6f4899db49348db623700a2d8)

I think it would be useful to add that mempool conflicted transactions are serialized as inactive transactions, so the distinction between TxStateMempoolConflicted and TxStateInactive only exists at runtime.

🤔 sr-gi reviewed a pull request: "test: fix intermittent failure in p2p_v2_earlykeyresponse"
(https://github.com/bitcoin/bitcoin/pull/29352#pullrequestreview-1854149947)
I wonder if we will not better off not having a simpler `initiate_v2_handshake` in this case and avoiding sync + async logic. Given we end up having to manually send raw messages and accessing `peer1` internal state anyway, I think it would be simpler to send everything manually:


```python
class TestEncryptedP2PState(EncryptedP2PState):
def __init__(self):
super().__init__(initiating=True, net='regtest')
self.magic_sent = False

def initiate_v2_handshake(self)
...

👍 theStack approved a pull request: "test: fix intermittent failure in p2p_v2_earlykeyresponse"
(https://github.com/bitcoin/bitcoin/pull/29352#pullrequestreview-1854169206)
Tested ACK 9642aefb81a9c87227eccf9997380024247ed1fc

Reproduced the issue and verified it's fix by adding an artificial delay in `initiate_v2_handshake`, as suggested in the PR description.
(With artificial delays of 3 seconds or more, the test fails again with an `IOError('Not connected')`, but this is expected as the TestNode closes the connection if there is no activity: `... [InactivityCheck] [net] socket no message in first 3 seconds ...`)

🤔 furszy reviewed a pull request: "Debug Console implementation of generate method"
(https://github.com/bitcoin-core/gui/pull/692#pullrequestreview-1852779591)
Executing `generate 1` on a node without a wallet leads to a program crash. And left some other review notes.

Will read the historical context for the `generate` command deletion before proceed.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473018733)
The function iterates over the entire input command string twice. It isn't really "fast".
Could remove this line altogether.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473029584)
There is an indentation issue here.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1472227098)
This should pass through the model.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473064260)
In case you retouch this; these functions should return a `util::Result<QString>` instead of a boolean. This way, the returned string can be emitted at the caller side (same as it is done for the RPC commands). This will improve the existing structure that has all functions returning true on all code paths.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473055299)
Same as above, this shouldn't be here. It is explaining the caller's workflow inside `executeConsoleGenerate`. There is no other RPC calls at this point.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473035544)
This shouldn't be here. The function's docstrings shouldn't explain the caller workflow behavior.
Something like "Executes gui-console-only commands" would be preferable.

💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1473052359)
To access the chain type, use the node interface. See [03d67301e081ecf3123372901b115ee5e29d7c79](https://github.com/furszy/bitcoin-core/commit/03d67301e081ecf3123372901b115ee5e29d7c79). So we don't violate the layers division.

💬 murchandamus commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1919410802)
Hey, I was taking a look at this PR. What is the status on this? Are we aiming to get this into 27.0? What about the two PRs, that @S3RK mentioned, they are open and in draft. Does this PR here depend on them?

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1473075595)
I'm not sure if this should be optional. Was thinking before about asserting here. Wdyt?

💬 epiccurious commented on pull request "net: enable v2transport by default":
(https://github.com/bitcoin/bitcoin/pull/29347#issuecomment-1919414747)
utACK

💬 mzumsande commented on pull request "test: fix intermittent failure in p2p_v2_earlykeyresponse":
(https://github.com/bitcoin/bitcoin/pull/29352#issuecomment-1919419940)
> I wonder if we will not better off having a simpler `initiate_v2_handshake` in this case and avoiding sync + async logic. Given we end up having to manually send raw messages and accessing `peer1` internal state anyway, I think it would be simpler to send everything manually:

Sounds reasonable to me, but I'd like to refer that to @stratospher, who I believe has plans to extend / rewrite the test anyway (see the WIP branch linked [here](https://github.com/bitcoin/bitcoin/pull/24748#issuecomm
...

📝 hebasto opened a pull request: "test: Drop `x` modifier in `fsbridge::fopen` call"
(https://github.com/bitcoin/bitcoin/pull/29357)
The MinGW-w64 toolchain links executables to the old msvcrt C Runtime Library that does not support the `x` modifier for the [`_wfopen()`](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen?view=msvc-170) function. It is possible to link to the new [UCRT](https://devblogs.microsoft.com/cppblog/introducing-the-universal-crt/) library instead. However, the latter is available on Windows 10 only, which breaks our release compatibility with the older Windows versions.


...

💬 achow101 commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1919425343)
ACK 27f260aa6e04f82dad78e9a06d58927546143a27

🤔 mzumsande reviewed a pull request: "test: p2p: adhere to typical VERSION message protocol flow"
(https://github.com/bitcoin/bitcoin/pull/29353#pullrequestreview-1854206288)
Concept ACK, I think the test framework should replicate the bitcoind logic (that, as a fun fact, has been in place since the patch #101 by Hal Finney)

💬 maflcko commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919441125)
> However, the latter is available on Windows 10 only, which breaks our release compatibility with the older Windows versions.

No? According to the link you provided:

> The Universal CRT is a component of the Windows operating system. It is included as a part of Windows 10, starting with the [January Technical Preview](http://windows.microsoft.com/en-us/windows/preview-download), and it is available for older versions of the operating system via Windows Update.

💬 maflcko commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919444440)
> Nevertheless, the second commit introduces an assertion in the AutoFile constructor.

Can you explain why this change makes sense? `AutoFile` already checks for nullptr.