💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426942102)
Er, the ugly thing is testmempoolaccept... since `AcceptMultipleTransactions` doesn't actually enforce the child-with-parents thing, we can have this right now:

```python
@cleanup
def test_v3_in_testmempoolaccept(self):
node = self.nodes[0]

self.log.info("Test that v3 inheritance is accurately assessed in testmempoolaccept")
tx_v2 = self.wallet.create_self_transfer(version=2)
tx_v2_from_v2 = self.wallet.create_self_transfer(utxo_to_spend=tx_v2["
...

💬 maflcko commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1856146425)
I was looking in moving boost signals2 into one translation unit (because it is so heavy and keeps the compiler busy), to only expose the stuff that is needed, but if it is possible to re-implement signals2 in 150 LOC, that seems preferable?

💬 fanquake commented on pull request "ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid unrelated failures":
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1856150718)
> We do not use any of other tools preinstalled via Homebrew.

Don't we use at least git, curl and Python? If any those are probably less-prone to breakage.

💬 LaurentMT commented on issue "Improving fee estimation accuracy":
(https://github.com/bitcoin/bitcoin/issues/27995#issuecomment-1856162406)
A minimalist implementation of the second idea ("look at the rate and feerate of transactions entering the mempool currently"): https://code.samourai.io/oxt/one_dollar_fee_estimator

From our observations, the estimator behaves rather well under current network conditions but as noted in the original post, no fee estimator should be used automatically without safeguards put in place.

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