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

💬 Sjors commented on pull request "Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#discussion_r1495576450)
b67b6ab3b69d45034b17f4704a1f3cedb07e9af6: It does not? `(requires Boost::Process)`

💬 fjahr commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1495836330)
I would either expect that the internally set option can not be overridden outside because the developers have decided that this test has to run this setting to keep good coverage no matter what the user thinks or that the test is skipped when the internal option is not compatible with what is provided from outside. Depends a bit on how important v1 test coverage is for us in the short term I think.

💬 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_r1495837621)
Here is a patch that merges the two processing loops into one (deduplicating some logic between them), and also makes the tail feerate explicit (even though only `FeeFrac{0, 1}` is used for now).

```patch
diff --git a/src/util/feefrac.cpp b/src/util/feefrac.cpp
index a970adedc30..569f32f4615 100644
--- a/src/util/feefrac.cpp
+++ b/src/util/feefrac.cpp
@@ -22,7 +22,7 @@ void BuildDiagramFromUnsortedChunks(std::vector<FeeFrac>& chunks, std::vector<Fe
}
}

-std::partial_ordering
...

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1495839770)
to make sure I'm not misreading, this is a bump of the default feerate value by 10x? Making sure since I don't see any tests that had to be changed.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-1954241752)
Rebased after #29441 landed which contains two commits from this PR.

💬 instagibbs commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1495847533)
I'm likely misreading this, but `DEFAULT_MAX_RAW_TX_FEE_RATE` doesn't seem to actually be used anywhere except `PSBTOperationsDialog::broadcastTransaction()`?

👋 maflcko's pull request is ready for review: "lint: Check for missing bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29408)

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1495861406)
taken, thanks

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#issuecomment-1954265365)
rebased