💬 brunoerg commented on pull request "fuzz: coinselection, improve `min_viable_change`/`change_output_size`":
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1856347794)
> Need to provide min_viable_change to the BnB result GetChange() function, not cost_of_change. @brunoerg

Got same crash with it.

```diff
diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp
index b35bf34db3..5a3250bdd7 100644
--- a/src/wallet/test/fuzz/coinselection.cpp
+++ b/src/wallet/test/fuzz/coinselection.cpp
@@ -128,7 +128,7 @@ FUZZ_TARGET(coinselection)
auto result_bnb = coin_params.m_subtract_fee_outputs ? util::Error{Untranslat
...

💬 maflcko commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1856365111)
> maybe rename `fs::u8path` to `fs::utf8path` for consistency

I think it is nice that it shadows the deprecated "constructor", but I am happy to review a follow-up. (Can be a scripted-diff, because no use of the deprecated `u8path` remain in the code)

💬 maflcko commented on pull request "guix: use GCC 12.3.0 to build releases":
(https://github.com/bitcoin/bitcoin/pull/27897#issuecomment-1856386504)
> Note that there is also non-determinism with GCC-11, i.e this branch https://github.com/fanquake/bitcoin/commits/test_11:

Did you also try gcc-13.2, which may be an alternative at this point?

💬 murchandamus commented on pull request "fuzz: coinselection, improve `min_viable_change`/`change_output_size`":
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1856393084)
It seems to me that the check in line 131 is simply wrong:

```
assert(result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0);
```

The function definition is `CAmount SelectionResult::GetChange(const CAmount min_viable_change, const CAmount change_fee) const`

So, instead of passing `coin_params.m_cost_of_change` and `0`, we should be passing `coin_params.min_viable_change` and `coin_params.m_change_fee`. Even just fixing the latter was insufficient, but when I replace
...

💬 fanquake commented on pull request "build: Require libc++-16 or later":
(https://github.com/bitcoin/bitcoin/pull/29077#issuecomment-1856394358)
> and OSS-Fuzz?

So won't this break as soon as we have code that requires libstdc++-16, given that OSS-Fuzz is still pinned to 15?

Are there no other projects using (some?) C++20 code on OSS-Fuzz?

💬 theuni commented on pull request "refactor: Remove gmtime*":
(https://github.com/bitcoin/bitcoin/pull/29081#issuecomment-1856399535)
Finally :)

Concept ACK.

💬 achow101 commented on pull request "Make bitcoin-tx replaceable value optional":
(https://github.com/bitcoin/bitcoin/pull/29022#issuecomment-1856400401)
ACK 98afe7866185ed4157ffc581763e11dc02fcbae0

🚀 achow101 merged a pull request: "Make bitcoin-tx replaceable value optional"
(https://github.com/bitcoin/bitcoin/pull/29022)

✅ achow101 closed an issue: "bitcoin-tx replaceable value should be optional, but isn't"
(https://github.com/bitcoin/bitcoin/issues/28638)

💬 theuni commented on pull request "refactor: Remove gmtime*":
(https://github.com/bitcoin/bitcoin/pull/29081#issuecomment-1856412027)
> util/time.cpp:60:24: error: ‘year_month_day’ in namespace ‘std::chrono’ does not name a type

It would have been too easy if it had just worked in the real world :p

💬 maflcko commented on pull request "refactor: Remove gmtime*":
(https://github.com/bitcoin/bitcoin/pull/29081#issuecomment-1856419623)
Jup, a bump to g++-11 should be fine, but currently not possible because the guix build is still on g++-10 🫠

💬 furszy commented on pull request "fuzz: coinselection, improve `min_viable_change`/`change_output_size`":
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1856422920)
> So, instead of passing coin_params.m_cost_of_change and 0, we should be passing coin_params.min_viable_change and coin_params.m_change_fee. Just fixing one or the other was insufficient, but when I replace both all my fuzz crashes pass.

Yeah, thats good.
It is because `cost_of_change`, the BnB upper bound, includes `change_fee` while `min_viable_change` does not.

💬 fanquake commented on pull request "guix: use GCC 12.3.0 to build releases":
(https://github.com/bitcoin/bitcoin/pull/27897#issuecomment-1856423621)
Yes, iirc. I'll get back to this now that it's becoming more of a blocker. I've bed sick of continually dealing with/having to track dow problems in gui and similar non-critical code, when it comes to these kinds changes.

💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1427157449)
done

💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#issuecomment-1856427821)
[d636e38 ](https://github.com/bitcoin/bitcoin/commit/d636e38d79a4c3950da91090b1f787163f11e24d)to 2a17ac3:
Renamed `GetExtraFullOutboundCount` to `GetFullOutboundDelta` since it can now return negative values, plust small doc change in the test.

💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1427158995)
done now!

💬 achow101 commented on pull request "test: add TestNode wait_until helper":
(https://github.com/bitcoin/bitcoin/pull/29070#issuecomment-1856431717)
ACK bf0f7dbec6590a54ec890e7a2ca5d85427995334

🚀 achow101 merged a pull request: "test: add TestNode wait_until helper"
(https://github.com/bitcoin/bitcoin/pull/29070)

✅ achow101 closed an issue: "test: Add TestNode wait_until helper"
(https://github.com/bitcoin/bitcoin/issues/29029)

🤔 furszy reviewed a pull request: "fuzz: coinselection, improve `min_viable_change`/`change_output_size`"
(https://github.com/bitcoin/bitcoin/pull/28372#pullrequestreview-1782561357)
Left a comment