💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1495633116)
> If you want to test a large number, then better check against TX_MAX_STANDARD_VERSION + 1?

This check already exists two lines below.

> Does this still look meaningful?

Yes, the goal is to check the edge cases (minimum and maximum possible number)

💬 sipa commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1495653173)
It'd look a bit more natural using `= 0xffffffff` or `= std::numeric_limits<uint32_t>::max()`.

💬 hebasto commented on issue "ci_native_asan: UndefinedBehaviorSanitizer: null-pointer-use qt/test/wallettests.cpp:424:25 in":
(https://github.com/bitcoin-core/gui/issues/796#issuecomment-1954008330)
@maflcko

Does https://github.com/bitcoin-core/gui/pull/797 fix this issue for you?

💬 maflcko commented on pull request "test: Recognize dialog object by name":
(https://github.com/bitcoin-core/gui/pull/797#issuecomment-1954016527)
lgtm, Thanks!

💬 dergoegge commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#issuecomment-1954024203)
I would like to see this in v27, can we add it to the milestone?

💬 fanquake commented on pull request "refactor: bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1954026863)
Note there are at least two defines missing from the scriptied diff (`_TIME_BITS` & `__MINGW_USE_VC2005_COMPAT`), if using Autoconf 2.72 , however that should not make a difference here. Looks like `WORDS_BIGENDIAN` is also missing, but that should als be ok.

💬 sipa commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1495673285)
Indeed, reverted (and also stuck closer to the former code).

✅ hebasto closed an issue: "ci_native_asan: UndefinedBehaviorSanitizer: null-pointer-use qt/test/wallettests.cpp:424:25 in"
(https://github.com/bitcoin-core/gui/issues/796)

🚀 hebasto merged a pull request: "test: Recognize dialog object by name"
(https://github.com/bitcoin-core/gui/pull/797)

💬 sipa commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1954030462)
@luke-jr There were a few expected encoding value tests, but I've expanded them significantly now.

💬 sipa commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1495685692)
Yeah, it means "fee fraction", though the name isn't perfect - it really represents any ratio where the numerator is an `int64_t` and the denominator is an `int32_t`, with a sort order that tie-breaks by putting larger denominators last.

I don't think Chunk is a good name, as we're using that term for a subset of transactions. A `FeeFrac` is related, but represents the aggregate feerate of a chunk (or really of any set of transactions, or even the difference between the fees/sizes of two sets
...

💬 maflcko commented on pull request "refactor: bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1954058149)
> Looks like `WORDS_BIGENDIAN` is also missing, but that should also be ok.

It is covered in #29408, in any case.

📝 BrandonOdiwuor opened a pull request: "doc: Fix gen-manpages to check build options"
(https://github.com/bitcoin/bitcoin/pull/29457)
### (!!! Disclaimer: This is an initial draft PR to get feedback on the approach)

Fixes https://github.com/bitcoin/bitcoin/issues/17506

gen-manapages.py should check enabled build options when generating docs

💬 maflcko commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1495735626)
```
$ FUZZ=coin_grinder_is_optimal UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/test/fuzz/fuzz /tmp/b

wallet/test/fuzz/coinselection.cpp:161:89: runtime error: implicit conversion from type 'CAmount' (aka 'long') of value -12 (64-bit, signed) to type 'size_type' (aka 'unsigned long') changed the value to 18446744073709551604 (64-bit, unsigned)

SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-c
...

💬 maflcko commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1495746878)
My understanding would be that the check returns an error to recommend to build with the component, when a component is missing, no?

💬 fjahr commented on pull request "doc: Assert that assumed-valid blocks are not fully valid in CheckBlockIndex()":
(https://github.com/bitcoin/bitcoin/pull/29355#issuecomment-1954125456)
> @fjahr i think drahtbot added that label because the commit starts with “doc:”

Yes, my question is why the "doc" is there in the name

👍 fanquake approved a pull request: "refactor: bitcoin-config.h includes cleanup"
(https://github.com/bitcoin/bitcoin/pull/29404#pullrequestreview-1890385608)
ACK 9d1dbbd4ceb8c04340927f5127195dc306adf3fc

🚀 fanquake merged a pull request: "refactor: bitcoin-config.h includes cleanup"
(https://github.com/bitcoin/bitcoin/pull/29404)

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1495827183)
Haven't tried, but I expect this will fix it:

```patch
// Ensure that each UTXO has at least an effective value of 1 sat
+ if (max_output_groups + max_spendable >= MAX_MONEY + group_pos.size()) break;
const CAmount eff_value{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, MAX_MONEY - max_spendable - max_output_groups + group_pos.size())};
```

💬 Sjors commented on pull request "Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1954224796)
tACK b67b6ab3b69d45034b17f4704a1f3cedb07e9af6

Tested on macOS 14.2.1, Windows 11 Home (using cross compiled Guix build) and Ubuntu 23.10.

I didn't test Windows native. It might be worth splitting off the last commit to get this merged faster. I'm assuming it's more important to get rid of boost process than it is to support native Windows builds.

Commit f6a3b5ef643d4993f0ae3faa0c5ff1ae62ad71d8 which switches the external signer code to use cpp-subprocess looks correct.

I perused the
...