💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2041068131)
> This is from an attempt to compile (no linking involved) the single blockfilter_tests.cpp file.

Does it compiles on the master branch for you?

💬 sipsorcery commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2041068497)
> @sipsorcery
>
> > I'm on Windows and using msvc but I did do a clean build but it didn't help.
>
> What VS / MSVC versions do you use?

Visual Studio (including msvc) updates get automatically pushed every couple of weeks. I'm generally always pretty close to the latest stable version.

Currently Visual Studio 2022 17.8.0

```
msbuild --version
MSBuild version 17.8.3+195e7f5a3 for .NET Framework
17.8.3.51904
```

💬 sipsorcery commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2041068780)
> > This is from an attempt to compile (no linking involved) the single blockfilter_tests.cpp file.
>
> Does it compile on the master branch for you?

No.

I only checked that after posting the compiler error message. The msvc compiler error isn't related to this PR, it's on master as well.

💬 tdb3 commented on pull request "p2p: gives seednode priority over dnsseed if both are provided":
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-2041082464)
re-ACK for 2842e51a246b162a586941184b7694f187d7aee7

The code adjustments made to bound scope and increase declaration/usage proximity look great to me (cleaner, more readable).

Built and ran all functionals (all passed).

Cleared datadir (with the exception of bitcoin.conf, chainstate, and blocks). Performed >1000 block download on signet to a reachable public node specified as a seednode. dnsseeds seemed to be avoided as expected.

```
2024-04-06T13:15:52Z dnsseed thread start
202
...

💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2041086383)
> The msvc compiler error isn't related to this PR, it's on master as well.

Could you please open a separate issue then?

FWIW, I have no compile errors for the master branch @ b5d21182e5a66110ce2796c2c99da39c8ebf0d72 using VS Community 2022 17.9.5. For example, the `test_bitcoin.exe` compiles for both `Debug` and `Release` configurations:
```
> msbuild --version
MSBuild version 17.9.8+b34f75857 for .NET Framework
17.9.8.16306
> py -3 build_msvc\msvc-autogen.py
> msbuild build_msvc\bi
...

💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1554621632)
`... + 32` is consistent with other `CKey::Set()` calls in our codebase:

https://github.com/bitcoin/bitcoin/blob/b5d21182e5a66110ce2796c2c99da39c8ebf0d72/src/bench/bip324_ecdh.cpp#L30

https://github.com/bitcoin/bitcoin/blob/b5d21182e5a66110ce2796c2c99da39c8ebf0d72/src/bench/ellswift.cpp#L20

https://github.com/bitcoin/bitcoin/blob/b5d21182e5a66110ce2796c2c99da39c8ebf0d72/src/key.cpp#L330

https://github.com/bitcoin/bitcoin/blob/b5d21182e5a66110ce2796c2c99da39c8ebf0d72/src/key.cpp#L393
...

📝 hebasto opened a pull request: "refactor, bench, fuzz: Drop unneeded `UCharCast` calls"
(https://github.com/bitcoin/bitcoin/pull/29820)
The `CKey::Set()` template function handles `std::byte` just fine: https://github.com/bitcoin/bitcoin/blob/b5d21182e5a66110ce2796c2c99da39c8ebf0d72/src/key.h#L105

Noticed in https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1546288181.

💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1554623656)
> If there is no need for it, it may be better to remove it.

Please see https://github.com/bitcoin/bitcoin/pull/29820.

💬 fanquake commented on pull request "ci: remove --with-asm=no (secp256k1) from MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/29742#issuecomment-2041113829)
Have run this successfully on all 4 MSAN jobs locally.

📝 hebasto opened a pull request: "fuzz: Some `test/fuzz/test_runner.py` improvements"
(https://github.com/bitcoin/bitcoin/pull/29821)
These changes are split from https://github.com/bitcoin/bitcoin/pull/29774 and can be beneficial on their own.

The new `BITCOINFUZZ` environment variable complements the already existing set of variables used by tests: https://github.com/bitcoin/bitcoin/blob/b5d21182e5a66110ce2796c2c99da39c8ebf0d72/test/functional/test_framework/test_framework.py#L238-L243

💬 hebasto commented on pull request "fuzz: Some `test/fuzz/test_runner.py` improvements":
(https://github.com/bitcoin/bitcoin/pull/29821#issuecomment-2041115220)
cc @maflcko @dergoegge

👍 BrandonOdiwuor approved a pull request: "assumeutxo: Fix -reindex before snapshot was validated"
(https://github.com/bitcoin/bitcoin/pull/29726#pullrequestreview-1984575158)
re-ACK b7ba60f81a33db876f88b5f9af1e5025d679b5be

🤔 BrandonOdiwuor reviewed a pull request: "test: Refactor fee calculation to remove satoshi_round function"
(https://github.com/bitcoin/bitcoin/pull/29566#pullrequestreview-1984578430)
Concept ACK moving from `float` to `Decimal`

📝 theStack opened a pull request: "test: remove immediate tx relay workaround in wallet_groups.py"
(https://github.com/bitcoin/bitcoin/pull/29822)
Reverts commit ab4efad51b9ba276ffeb6871931e13772493f7cc (PR #26970). This workaround is not needed anymore, as since #27114 the test sets the noban permission for both in- and outbound connections via the `noban_tx_relay` setting, and we don't have to rely on this topology hack anymore. See commit c985eb854cc86deb747caea5283c17cf51b6a983 (kudos to brunoerg!).

Can be tested by executing `$ time ./test/functional/wallet_groups.py` both on master and PR and verifying that the execution time is r
...

💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1554638887)
Thanks! Removed.

💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2041136905)
The branch has been reworked to address @maflcko's comments.

> ... but bool return value + exceptions doesn't make sense, does it?

Whether [`std::istream::read()`](https://en.cppreference.com/w/cpp/io/basic_istream/read) throw or not depends on how [`exceptions()`](https://en.cppreference.com/w/cpp/io/basic_ios/exceptions) is set. The suggested code does not use exceptions.

💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1554639703)
Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2041136905).

💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1554639779)
[Reworked](https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2041136905).

💬 hebasto commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1554639810)
[Reworked](https://github.com/bitcoin/bitcoin/pull/29774#issuecomment-2041136905).

💬 fanquake commented on pull request "build: Enable fuzz binary in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29774#discussion_r1554640585)
I don't really think we want a reversion here (or in addition), and if we were reverting shouldn't the `__GNUC__` case go away?

In any case, I'd rather we just not define this for MSVC.