💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1424216465)
Pulled into a helper function and reused :+1:

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1852315761)
Fixed the fuzzer crashes in https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1828351782 and https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1835850051 (thanks)

💬 fanquake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1852336494)
Added this to #29011 for backporting to 26.x.

💬 mzumsande commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1424240319)
thanks, removed the comment.

💬 mzumsande commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1852356884)
> Playing devil's advocate: could there be any noticeable drawbacks of always trying with v2 first? I'm thinking of destination nodes behind firewalls with deep packet inspection which block ips from further connections if they don't follow the protocol (i.e. for bitcoin v1 connections, initiating with a version message). If that's the case, the user would only have the option to disable v2transport in general, or use the `addnode` RPC instead. Not sure how realistic that "i want to connect via
...

💬 theuni commented on pull request "Replace non-standard CLZ builtins with c++20's bit_width":
(https://github.com/bitcoin/bitcoin/pull/29057#issuecomment-1852375970)
Thanks @theStack and @hebasto for the quick review.

@fanquake sounds good, will push a commit to do the switch once we see some benchmarks.

⚠️ hebasto opened an issue: "build: GCC 10.5 fails to compile `test_bitcoin` using Boost.Test 1.71"
(https://github.com/bitcoin/bitcoin/issues/29063)
On fresh Ubuntu 20.04, with dependencies installed via `apt`:
```
$ ./configure CC=gcc-10 CXX=g++-10
$ make -C src test/test_bitcoin
make: Entering directory '/bitcoin/src'
CXX test/test_bitcoin-main.o
In file included from /usr/include/boost/test/included/unit_test.hpp:29,
from test/main.cpp:10:
/usr/include/boost/test/impl/test_tools.ipp: In member function 'void boost::test_tools::tt_detail::print_log_value<const wchar_t*>::operator()(std::ostream&, const wchar
...

💬 hebasto commented on issue "build: GCC 10.5 fails to compile `test_bitcoin` using Boost.Test 1.71":
(https://github.com/bitcoin/bitcoin/issues/29063#issuecomment-1852404642)
Quick googling shows:
- upstream bug -- https://github.com/boostorg/test/issues/249
- upstream fix -- https://github.com/boostorg/test/pull/252

Should we update the minimum support Boost version then?

📝 achow101 converted_to_draft a pull request: "wallet: reenable sethdseed for descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/29054)
Enable `sethdseed` for descriptor wallets. To be able to use `createwalletdescriptor` with the other address types, we need a way to change the wallet extended key, and so `sethdseed` has been updated and enabled for descriptor wallets. As with legacy wallets, when called without parameters, it will generate a new random master key for the wallet. It can also take a xprv and set that as the master key. It still takes a BIP 32 seed as WIF or as hex as we do for legacy wallets. The seed will be tr
...

💬 maflcko commented on issue "build: GCC 10.5 fails to compile `test_bitcoin` using Boost.Test 1.71":
(https://github.com/bitcoin/bitcoin/issues/29063#issuecomment-1852417369)
> Should we update the minimum support Boost version then?

Sounds good. I don't see an alternative.

💬 hebasto commented on issue "build: GCC 10.5 fails to compile `test_bitcoin` using Boost.Test 1.71":
(https://github.com/bitcoin/bitcoin/issues/29063#issuecomment-1852420355)
> > Should we update the minimum support Boost version then?
>
> Sounds good. I don't see an alternative.

Going to do more thorough tests before submitting a PR.

💬 1ma commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1852422627)
> > The original Satoshi client didn't allow embedding data onchain, this has always been an exploit.
>
> Bitcoin was released with no standardness rules at all. You absolutely could embed data onchain. You could even publish arbitrary data to the UTXO set easily.

That was poor wording, but was precisely my point.

Satoshi introduced OP_RETURN as a mechanism to prove that some sats were provably unspendable. Then it turned out that the Bitcoin interpreter was too lax and OP_RETURN could
...

💬 dergoegge commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1852439055)
```
$ echo "/v//tk/aAYxsz8/Pz8/PAAAAAAB2AAYAAADPz8/Pz8/Pz8/Pz8/Pz8/Pz8/Pz8/Pz88AAABAAgAAAL35AAAABwIBAIwAAABA0gH5ACMAAAAAAAAAAAAAAAAAAAAADgAAAAAAAAAAAAFBWwAACD/gPp66urq6uro=" | base64 --decode > tx_package_eval-e9f61e34e32c669558b51daaec0c5c3780377b37.crash
$ FUZZ=tx_package_eval ./src/test/fuzz/fuzz tx_package_eval-e9f61e34e32c669558b51daaec0c5c3780377b37.crash
test/util/txmempool.cpp:132 CheckMempoolV3Invariants: Assertion `entry.GetTxSize() <= V3_CHILD_MAX_VSIZE' failed.
```

🤔 instagibbs requested changes to a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1777711045)
`test_v3_ancestors_package` checked two failing conditions, which allowed the one condition(too heavy child) to slip through

```
diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py
index 837846970f..ee3cdd4cd4 100755
--- a/test/functional/mempool_accept_v3.py
+++ b/test/functional/mempool_accept_v3.py
@@ -222,58 +222,69 @@ class MempoolAcceptV3(BitcoinTestFramework):
node = self.nodes[0]
self.log.info("Test that below-min-relay-fee
...

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1424133394)
extra line

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1424137225)
isn't this already covered by CheckV3Inheritence?

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1424133256)
unused addition?

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1424141970)
`Assume(IsChildWithParents())` here makes sense

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1424190533)
let's check the error messages here and elsewhere to prevent test regressions:
`'error': 'v3-nonstandard, tx 1399d1fad241d2da8057b0eb29524e75b8415dedc26a536d8a0cfa72f49f9747 would have too many ancestors'}`

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1424302431)
`V3_CHILD_MAX_VSIZE` is the wrong thing, obviously, but it does need to be checked, otherwise a package size two doesn't fail when the child is too big