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

💬 hebasto commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919450825)
> > Nevertheless, the second commit introduces an assertion in the AutoFile constructor.
>
> Can you explain why this change makes sense? `AutoFile` already checks for nullptr.

The added assertion makes the issue with the unsupported `x` modifier explicit:

> The `streams_tests/xor_file` test will fail without the first commit.

🤔 furszy reviewed a pull request: "rpc: Drop migratewallet experimental warning"
(https://github.com/bitcoin/bitcoin/pull/28037#pullrequestreview-1854247504)
> 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?

#28574 is in draft because it is the parent PR. It consolidates the entire work in a single location, allowing tests and benchmarks to be performed against the end goal as well.
This work was decoupled into three independent PRs: #28894, #28987, #26836. There is no specific order req
...

💬 maflcko commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919453909)
It is already explicit and will fail on master without the first commit:

```
unknown location(0): fatal error: in "streams_tests/xor_file": std::ios_base::failure[abi:cxx11]: AutoFile::write: file handle is nullptr: iostream error

💬 fanquake commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919469009)
Concept NACK. I don't think we should remove test coverage from all other platforms, just to accomodate Windows. In the worse case this should just have a Windows paths with the modified modifiers, with documentation explaining why?

💬 furszy commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1919470797)
Also, we do have other bug fixes that should be included in v27 to deprecate this warning.
See #28976, #28868, #29112. Would be nice to bring more attention to them.

💬 maflcko commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1473125056)
nit: Any reason to add `__func__` to this log, when it previously wasn't there? If someone is interested in the exact source location, they can enable the corresponding log option. If this is needed for context, it could make more sense to clarify the log message itself, so that it is unique and clear without requiring the source code as context.

👍 stickies-v approved a pull request: "refactor: Fix prevector iterator concept issues"
(https://github.com/bitcoin/bitcoin/pull/29275#pullrequestreview-1854275203)
ACK fad74bbbd0ab4425573f182ebde1b31a99e80547

💬 hebasto commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919481274)
> Concept NACK. I don't think we should remove test coverage from all other platforms, just to accomodate Windows.

It is not about Windows as a platform. It is about Windows users who download the Bitcoin Core release binaries. We should not treat them in a discriminatory way.