💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1425704039)
no straight forward coverage of this I think, so:

```suggestion

@cleanup
def test_v3_package_inheritence(self):
self.log.info("Test that v3 inheritence is checked within package")
node = self.nodes[0]
self.restart_node(0, extra_args=["-datacarriersize=1000"])
tx_v3_parent = self.wallet.create_self_transfer(
fee_rate=0,
target_weight=4004,
version=3
)
tx_v2_child = self.wallet.create_self_t
...

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1425705028)
```suggestion
self.test_v3_ancestors_package_and_mempool()
self.test_v3_package_inheritence()
```

💬 theuni commented on pull request "Replace non-standard CLZ builtins with c++20's bit_width":
(https://github.com/bitcoin/bitcoin/pull/29057#issuecomment-1854487602)
I can reproduce a slight slowdown here. I added a bench that demonstrates the difference, then the original commit, then the removal.

Both libc++ and libstdc++ implement this in terms of the same built-ins we were using before, so I find this surprising. I'd hate to find that the c++ism's have a cost.

Going to convert this to a draft while I look into it.

👍 pinheadmz approved a pull request: "Make bitcoin-tx replaceable value optional"
(https://github.com/bitcoin/bitcoin/pull/29022#pullrequestreview-1780261073)
ACK 98afe7866185ed4157ffc581763e11dc02fcbae0

Great work Kashif!
Built and ran all tests locally. Confirmed that some new tests fail without the patch, and additional tests were added that expand coverage for the `bitcoin-tx` utility with "replaceable" option. I also played with the updated utility on the command line, and confirmed that this patch closes the issue linked in the PR description. 👍

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash:
...

🤔 ryanofsky reviewed a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-1780244193)
Updated 9ba4dc41fd405a008e9badc4092ef1905f534b96 -> 414546dcc455fb5f5f8e2c4591b2df1f16863cdc ([`pr/rmutil.7`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.7) -> [`pr/rmutil.8`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/rmutil.7..pr/rmutil.8)) adding new commit 18a9f203bba1ad551bdea0d0580b3e07c3715054 to break up TransactionError type and remove PSBT codes ll

💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1425731507)
re: https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1425022656

> I was mostly thinking that it doesn't make sense for `TransactionErrorString()` in common even though it depends on `TransactionError` which is in node, and node depends on common, not vice-versa? I wasn't expecting paragraphs of response :)

Yeah sorry thanks for pinning down the actual concern. Before you just said it "seems odd" so I felt like I needed to justify my entire thought process.

In any case, both the
...

📝 brunoerg opened a pull request: "fuzz: set `m_fallback_fee` and `m_fee_mode` in `wallet_fees` target"
(https://github.com/bitcoin/bitcoin/pull/29076)
`m_fallback_fee` and `m_fee_mode` are used in `GetMinimumFeeRate` but we're not setting any value for them in `wallet_fees` target. That's the reason fuzzing is never reaching the following code:


This PR fixes it.

📝 theuni converted_to_draft a pull request: "Replace non-standard CLZ builtins with c++20's bit_width"
(https://github.com/bitcoin/bitcoin/pull/29057)
Split out of #28674

Note that we can't yet drop our configure checks because we pass the results on to minisketch. I've opened a PR for that upstream here: https://github.com/sipa/minisketch/pull/80

fanquake [suggested](https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1422215535) that we simply replace our `CountBits` call-sites with uses of `std::bit_width` directly and just drop the tests and fuzzers. I agree with that, but I wanted to allow our tests/fuzzers to run with this ch
...

💬 maflcko commented on pull request "bench: wallet, fix change position out of range error":
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1854610666)
> This seems important to fix to me.

I tried on current master with `clang-13` and `valgrind` on Jammy, but it seems to pass.

Maybe it is a bug in your g++?

```
# git log -1 && valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection
commit 9f0f83d6509a214b827f5110c0f857b494ae854c (HEAD, origin/master, origin/HEAD)
Merge: 019ec8a 37c75c5
Author: Ava Chow <github@achow101.com>
Date: Wed Dec 13 12:38:11 2023 -0500

Merge bitcoin/bitcoin#29065
...

💬 mzumsande commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1854630701)
> What do you think?

To get an estimate about performance, I wrote a simple benchmark, see https://github.com/mzumsande/bitcoin/commit/22f98d6f54042b3e8ee41889ceb362f6355e4fa0. On my computer (neither particularly fast nor slow) I get `~1000op/s` per wtxid to call `ShouldFanoutTo()` for all 120 peers, so a batch of 3000 txs should take ~3s time in `ShouldFanoutTo`.
So I think that caching could make sense.

I wonder if there is there a good way to restrict this map to wtxids that are stil
...

👍 ryanofsky approved a pull request: "refactor: Remove pre-C++20 code, fs::path cleanup"
(https://github.com/bitcoin/bitcoin/pull/29040#pullrequestreview-1780346500)
Code review ACK 856c88776f8486446602476a1c9e133ac0cff510

I suggested a scripted diff followup, but all these changes seem good, and the followup could happen later. I am also a little disturbed that 856c88776f8486446602476a1c9e133ac0cff510 doesn't begin with `fa` but I think I can get over it.

💬 ryanofsky commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1425838080)
In commit "ArgsManager: return path by value from GetBlocksDirPath()" (856c88776f8486446602476a1c9e133ac0cff510)

This change looks ok, but it seems like the clang-18 warning is a false alarm. We know know m_cached_blocks_path will never change after it is written, and we always acquire the cs_args lock before reading m_cached_blocks_path so we can be sure that by the time we do read it we are necessarily seeing the latest value. I don't see how there could be a race condition here, because it
...

💬 ryanofsky commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1425796516)
In commit "refactor: Remove pre-C++20 fs code" (fa3d9304e80c214c8b073f12a7f4b08c5a94af04)

re: https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1422242907

> If a u8string was returned here, UniValue would have to be updated to take it as well, in which case the conversion would need to happen there.

In theory, no conversions are actually required, right? In theory UniValue could use u8string instead of string internally, and other parts of the codebase could too, so nothing woul
...

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1425860646)
> Nit in the last commit: Shouldn't new node code be in the node namespace? Otherwise it will have to be touched again when it is moved into the node namespace later?

This is true, but it is not different from other init.h methods. Logically init.h and init.cpp belong in node subdirectory and should use the node namespace, so I didn't want to make this one function stand out. This seemed like the most straightforward approach for now.

🤔 sipa reviewed a pull request: "p2p: Fill reconciliation sets (Erlay)"
(https://github.com/bitcoin/bitcoin/pull/28765#pullrequestreview-1780420519)
Not a full review; mostly performance improvement suggestions as I saw there was a suggestion to cache things. Before considering that, it may be worth benchmarking if that's still relevant with these review comments addressed.

💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425854190)
Constructing the full set of fanout candidates any time you want to consider relaying anything to any node has O(n^2 log n) scaling (you'll call `GetFanoutTargets` O(n) times, and each does O(n log n) work going over all nodes), I believe.

That may or may not matter, but I'd consider at least one of these variants:
* Cache the `GetFanoutTargets` result per transaction as discussed elsewhere in this PR, making it O(n log n) only.
* Instead of computing an `std::set<NodeId>` in `GetFanoutTarg
...

💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425857575)
Nit: `const size_t targets_size = integral_part + add_extra`.

💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425842632)
The `deterministic_randomizer_with_wtxid` variable could be `const CSipHasher&` here (to make it clear that this object is only ever used as a starting point for (independent) hashes).

💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425845551)
I believe it would be significantly faster to make this an `std::vector<std::pair<uint64_t, NodeId>>`, `.reserve()` it once, fill it up like in the loop below, and then `std::sort` it in-place using `cmp_by_key`. This has far better memory locality, less allocation overhead, and ought to have the same asymptotic complexity (O(n log n)).

💬 sipa commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1425862263)
Pass `indexed_state` by reference. This is creating a copy of the entire `TxReconciliationState` for every element in `m_states`.

`[](const auto& indexed_state) { ... }` works.