💬 theuni commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1856193772)
@maflcko IIRC I wanted to fix #26442 before going forward with the replacement, as that makes the implementation more straightforward and greatly simplifies testing. Any interest in taking a look there?

👍 TheCharlatan approved a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-1782050983)
Re-ACK 3c91202c5a03c57c9f83186de9d3caf337b78d45

💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1426859564)
This comment is confusing, aren't we going from `BERKELEY` to `BERKELEY_RO`?

💬 hebasto commented on pull request "ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid unrelated failures":
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1856204103)
> Don't we use at least git, curl and Python?

Right. Here is the `brew list -1` command output for the image version `20231025.2`:
```
ant
aom
apr
apr-util
argon2
aria2
aspell
autoconf
aws-sam-cli
azure-cli
bazelisk
berkeley-db
bicep
brotli
c-ares
ca-certificates
cairo
carthage
cffi
cmake
composer
curl
fontconfig
freetds
freetype
gcc
gcc@11
gcc@12
gd
gdbm
geckodriver
gettext
gh
giflib
git
git-lfs
glib
gmp
gnu-tar
gnupg
gnutls
gradle
graphite2
harfb
...

📝 maflcko opened a pull request: "refactor: Remove gmtime*"
(https://github.com/bitcoin/bitcoin/pull/29081)
Now that the `ChronoSanityCheck` has passed for everyone with C++17 and is guaranteed by C++20 to always pass, remove it.

Also, remove `gmtime_r` and `gmtime_s` and replace them with `year_month_day`+`hh_mm_ss` from C++20.

💬 maflcko commented on pull request "refactor: Replace GetTimeMicros by SystemClock":
(https://github.com/bitcoin/bitcoin/pull/27233#discussion_r1427019264)
Fixed in https://github.com/bitcoin/bitcoin/pull/29081

💬 nafisalawalidris commented on issue "test: Write assumeutxo tests":
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1856297893)
Hello @maflcko, I'm interested in working on this issue and adding tests for the AssumeUtxo feature. I will follow the guidance provided in the issue description and the template for writing tests. I will keep you posted on my progress and submit a pull request once the tests are ready.

📝 maflcko converted_to_draft a pull request: "refactor: Remove gmtime*"
(https://github.com/bitcoin/bitcoin/pull/29081)
Now that the `ChronoSanityCheck` has passed for everyone with C++17 and is guaranteed by C++20 to always pass, remove it.

Also, remove `gmtime_r` and `gmtime_s` and replace them with `year_month_day`+`hh_mm_ss` from C++20.

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