💬 fanquake commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#discussion_r1475966162)
> If the new runtime is available in guix, I'd say it would be better to just use it, instead of patching the code. Or is there some downside I am missing?

I agree that we should move to the new runtime. I assume the downside is possibly dropping support for Windows 7, but I don't see why that would be a problem.

💬 maflcko commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#discussion_r1475966908)
> > The other info seems a bit overkiil, I'm also not sure if it's correct. From a quick look, if you choose the runtime you want, when you compile mingw-w64, there's no need for any GCC flag.
>
> If the new runtime can be linked in guix builds, I'd say it would be better to use it, instead of patching the code. Or is there some downside I am missing?

💬 maflcko commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#discussion_r1475969263)
According to the docs, if it is missing, it can be installed via "Windows Update": https://devblogs.microsoft.com/cppblog/introducing-the-universal-crt/ . IIUC it is shipped by default on Windows 10.

👍 stickies-v approved a pull request: "refactor: Fix timedata includes"
(https://github.com/bitcoin/bitcoin/pull/29361#pullrequestreview-1859084735)
ACK fad0fafd5aca699cfab7673f8eb18211139aeb18

💬 stickies-v commented on pull request "refactor: Fix timedata includes":
(https://github.com/bitcoin/bitcoin/pull/29361#discussion_r1475972375)
nit: iwyu suggests

```
chain.h should add these lines:
namespace Consensus { struct Params; }

chain.h should remove these lines:
- #include <consensus/params.h> // lines 10-10
```

💬 hebasto commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#discussion_r1475973211)
> if you choose the runtime you want, when you compile mingw-w64

Yes, there is `--with-default-msvcrt=ucrt` configuration option.

💬 dooglus commented on issue "New crash in v26.0":
(https://github.com/bitcoin-core/gui/issues/785#issuecomment-1923669190)
It crashed again. This crash looks the same as the last one:

```
[Thread 0x7fffea7fb6c0 (LWP 2748) exited]
[Thread 0x7fffeaffc6c0 (LWP 2747) exited]

Thread 1 "bitcoin-qt-v26." received signal SIGSEGV, Segmentation fault.
QSortFilterProxyModelPrivate::build_source_to_proxy_mapping (
proxy_to_source=..., source_to_proxy=..., this=this@entry=0x555557a7d520)
at ../../include/QtCore/../../src/corelib/tools/qarraydata.h:61
61 ../../include/QtCore/../../src/corelib/tools/qarray
...

💬 theStack commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1475987895)
Sorry for the late reply. I think the exact order of the checks doesn't really matter here from a performance perspective, since in the typical case (if the passed pubkey is valid) all three of them are executed anyway. Also, if users pass in something that has a different format like e.g. a Bitcoin address, the error message "pubkey has to be in hex" seems to be more useful than "pubkey has the wrong length".

💬 delta1 commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-1923691732)
Concept ACK dddd56c6eecfb0e0f53874355ee821f5bd6af556

Looks like a nice code change to me. "previous releases" CI is turning up an issue in script/interpreter

```
script/interpreter.cpp: In function ‘bool EvalChecksigPreTapscript(const valtype&, const valtype&, prevector<28, unsigned char>::const_iterator, prevector<28, unsigned char>::const_iterator, unsigned int, const BaseSignatureChecker&, SigVersion, ScriptError*, bool&)’:
script/interpreter.cpp:325:44: error: no matching function f
...

💬 delta1 commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-1923698793)
> CI is turning up an issue in script/interpreter

looks like a few issues in that file

💬 maflcko commented on pull request "refactor: Fix timedata includes":
(https://github.com/bitcoin/bitcoin/pull/29361#discussion_r1475998235)
Going to leave removal of further includes to a follow-up

💬 fanquake commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#discussion_r1476007817)
Yea. So I don't see why anything GCC related is required, or needs to be mentioned here.

🤔 BrandonOdiwuor reviewed a pull request: "test: Assumeutxo with more than just coinbase transactions"
(https://github.com/bitcoin/bitcoin/pull/29354#pullrequestreview-1859156221)
Concept ACK

💬 theStack commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1923734576)
Concept ACK

💬 brunoerg commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r1476030643)
In b1b74a13adb8e943d90fbe56c4a706b0ae91335a: nit: I don't think to set network active here is a must, we could fuzz it with both network active and inactive.

💬 maflcko commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-1923782707)
@delta1 This is an issue in the C++ standard library. Basically, the standard assumes that `element_type` and `value_type` types are never provided by an iterator type at the same time. This was fixed in https://cplusplus.github.io/LWG/issue3446 and https://github.com/gcc-mirror/gcc/commit/186aa6304570e15065f31482e9c27326a3a6679f

💬 BrandonOdiwuor commented on issue "test: RPC coverage check doesn't work?":
(https://github.com/bitcoin/bitcoin/issues/27593#issuecomment-1923784437)
I have reviewed the issue and it's evident that the wallet RPCs are indeed absent in the generated `rpc_interface.txt` file. This observation aligns with the analysis provided by @kouloumos.

The root cause lies in the invocation of `start_node(...)` within `create_cache.py`, where the `write_all_rpc_commands(...)` function is called. This occurs during the creation of the cache on a node that lacks wallet functionality. Consequently, the wallet RPCs are not included in the generated `rpc_int
...

💬 brunoerg commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r1476052411)
In d923b86264df93ad86c7bd557b9970e156585dc7: You could use `ConsumeService` into `ConsumeServiceVector`.

```diff
diff --git a/src/test/fuzz/util/net.h b/src/test/fuzz/util/net.h
index a97017555d..78e61b51d9 100644
--- a/src/test/fuzz/util/net.h
+++ b/src/test/fuzz/util/net.h
@@ -102,8 +102,7 @@ inline std::vector<CService> ConsumeServiceVector(FuzzedDataProvider& fuzzed_dat
const size_t size = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, max_vector_size);
ret.reserv
...

💬 BrandonOdiwuor commented on issue "test: RPC coverage check doesn't work?":
(https://github.com/bitcoin/bitcoin/issues/27593#issuecomment-1923793691)
@kouloumos what do you mean by?
> we can avoid this issue, but just a reordering of cache-creation<>coverage-initialization pushes the issue to the next coverage-initialization which in that case will be the first test of the test-runner. If the node in that first test doesn't have a wallet, the issue remains.

Doesn't the coverage-initialization happen inside cache-creation. Do you mean there should be a coverage-initialization (generation of the rpc_interface.txt) before `create_cache.py`
...

💬 delta1 commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-1923798641)
@maflcko interesting, thanks for the context. does that mean that the previous releases CI job needs to be changed to user a newer gcc?