💬 theuni commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1423020120)
> I think I'd prefer to leave this with its comment. Note that we don't use `std::counl_zero` directly, we want kinda the opposite: `(sizeof(x) * 8 )- std::countl_zero(x);`

Actually, as it turns out, [std::bit_width](https://en.cppreference.com/w/cpp/numeric/bit_width) is exactly what we want (and it's even described in terms of `countl_zero`). I'll see if it works to use your suggestion with that function instead.

💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1423022711)
Ah oops my bad. I was backporting my proposed refactor from https://github.com/bitcoin/bitcoin/pull/28985/commits/5b8c165c2ae0448b802c0c4907303d016f301f0a. I use a feerate of 3000 ṩ/kvB there.

🤔 amitiuttarwar reviewed a pull request: "doc/reduce-traffic: update/clarify max outbound connection count"
(https://github.com/bitcoin/bitcoin/pull/29052#pullrequestreview-1776011334)
ACK d58f89d355faf46b68d0ff8095699d0aff41959c

💬 glozow commented on pull request "[WIP] p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#issuecomment-1850749413)
Rebased and fixed CI. This is in draft because I'm focusing on v3 stuff, can be reviewed for its approach.

🤔 amitiuttarwar reviewed a pull request: "addrman, refactor: improve stochastic test in `AddSingle`"
(https://github.com/bitcoin/bitcoin/pull/27319#pullrequestreview-1776020630)
ACK e064487ca28c12ba774c2f43a3c7acbdb1a278c9

💬 glozow commented on pull request "test: Add and use option for tx-version in MiniWallet methods":
(https://github.com/bitcoin/bitcoin/pull/28972#issuecomment-1850757594)
Nice, I shall cherry-pick this

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1423035255)
thanks :+1:

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1850768688)
I've pushed a solution for the downgrade and encrypt issue. This will store an extra boolean in `ACTIVEHDKEY` which indicates whether the wallet was encrypted at the time that record was written. Then, when we detect that the wallet is now encrypted, but the `ACTIVEHDKEY` was written when the wallet was unencrypted, we will run the upgrade again to detect the new hd key.

For wallets that are encrypted with software supporting the record, the encryption status flag will be updated when the new
...

👍 ryanofsky approved a pull request: "depends: fix libmultiprocess build on aarch64"
(https://github.com/bitcoin/bitcoin/pull/28846#pullrequestreview-1776000110)
Code review ACK 965d38d0ab2cfb9b40827e422f354a061fa31209. Left a suggestion to simplify the second commit, but it is not important and this change looks good as.

💬 ryanofsky commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1423019899)
In commit "depends: always install capnp to /lib" (6d7e71ed47f9f540d981d5433b8f4efcd0767641)

I think probably it would be good revert this line and only make this change in `capnp.mk` not `native_capnp.mk` since the PKG_CONFIG_PATH mentioned in the comment above is used to find cross-compiled dependencies, not native dependencies, so the reasoning in the comment doesn't really apply here. Reverting this change would also make the native_capnp package definition simpler and more consistent wit
...

💬 ryanofsky commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1423040107)
re: https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1417239636

In commit "depends: build libmultiprocess with position independant code" (965d38d0ab2cfb9b40827e422f354a061fa31209)

> Yes, the switch to CMake in #28856 didn't fix this issue.

I'm still not exactly clear why this change is needed here and also why `--with-pic` options seem to be used for many other packages as well. But at least after #28856 the PIC option is used more consistently, and it should cause no harm in
...

💬 brunoerg commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1423048346)
Perhaps the comment is not wrong, the parameter should be updated. Doesn't it?

💬 amitiuttarwar commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#issuecomment-1850781513)
approach ACK

> (the address manager interface does not support removing addresses at the moment and adding this seems to be a bigger effort)

since right now the only way addresses are kicked off is triggered by a collision, removing addresses from addrman would require introducing new test-only code paths. in addition to being a bigger effort, it would increase the surface area of potential issues in the future. eg. if someone tried to use it with mainnet code. when I was learning addrman
...

📝 achow101 opened a pull request: "Descriptor sethdseed"
(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
...

💬 achow101 commented on pull request "wallet: rpc to add automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/25907#issuecomment-1850793355)
I've split the `sethdseed` changes into #29054

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-1850801317)
@sdaftuar should be ready for another look

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1423076943)
Looks like it's broken, see this test:
```
diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py
index 769b177cf523..713a8cb7406b 100755
--- a/test/functional/mempool_accept_v3.py
+++ b/test/functional/mempool_accept_v3.py
@@ -229,11 +229,42 @@ class MempoolAcceptV3(BitcoinTestFramework):
tx_replacer = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_confirmed, fee_rate=DEFAULT_FEE * 10)
self.check_mempool([tx_replacer
...

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1423081068)
that's the known topology issue IIUC: https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1409842026

📝 achow101 opened a pull request: "tests, bench: Fix issue with CWalelt::LoadWallet() being called in the wrong places "
(https://github.com/bitcoin/bitcoin/pull/29055)
`CWallet::LoadWallet()` expects to be called after a `CWallet` is constructed, but before any of its member functions called. Doing so invalidates pointers which causes issues with some PRs and branches that I am working on. This was being used incorrectly in a few tests and benchmarks, resulting in segfaults.

As a precaution for this kind of issue in the future, I've also added a few asserts to `LoadWallet()` so that developers will notice when it is used incorrectly.

As similar issue was
...

💬 sdaftuar commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1423084047)
Should this be "> 2"?