💬 achow101 commented on pull request "bench: wallet, fix change position out of range error":
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1854429077)
> This pull does fix the crash but I am still getting valgrind errors when executing the benchmark

Unable to replicate this, so probably a separate issue.

🚀 achow101 merged a pull request: "bench: wallet, fix change position out of range error"
(https://github.com/bitcoin/bitcoin/pull/29065)

✅ achow101 closed an issue: "Benchmark WalletCreateTxUsePresetInputsAndCoinSelection crashes due to #25273"
(https://github.com/bitcoin/bitcoin/issues/29061)

💬 aureleoules commented on pull request "Replace non-standard CLZ builtins with c++20's bit_width":
(https://github.com/bitcoin/bitcoin/pull/29057#issuecomment-1854441665)
So the benchmark results are available now but they are pessimistic on a few benchs (WalletCreateTxUseOnlyPresetInputs, WalletAvailableCoins and PrevectorClearNontrivial).

I ran the benchmarks locally without cachegrind and there does not seem to be any variation between master and the pull.
But when I run them with cachegrind I notice the same variation as seen on corecheck.

This may be due to a limitation of cachegrind because it cannot emulate L2 cache and so results may be pessimist?
...

💬 1440000bytes commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1854442040)
Since I commented here with disagreement to approach: https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1846220961

I am suggesting an alternative approach and its up to PR author if he wants to make changes based on it:

1. Do not mix everything. Keep OP_RETURN separate and provide a config option where users can decide limits for data in witness which is disabled by default.

2. Add a disclaimer in the PR description that you are the CTO of a mining pool and this pull request ca
...

💬 aureleoules commented on pull request "bench: wallet, fix change position out of range error":
(https://github.com/bitcoin/bitcoin/pull/29065#issuecomment-1854461848)
> Unable to replicate this, so probably a separate issue.

I can consistently replicate the valgrind errors on a fresh new ubuntu docker container:

```
Ubuntu 22.04.3 LTS
g++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
valgrind-3.18.1

Using :
./configure --enable-bench --disable-tests --disable-gui --disable-zmq --disable-fuzz --enable-fuzz-binary=no
valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection
```

These errors do not appear in the commit 2
...

🤔 instagibbs reviewed a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1780186265)
more plausibly correct now, fuzzer is passing if the fee check is disabled

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1425695018)
Might be worth stressing that this is the only place where in-package inheritance is checked

"Necessary but insufficient"

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1425701016)
```suggestion
const auto& parents = entry.GetMemPoolParentsConst();
Assert(parents.begin()->get().GetTx().nVersion == 3);
}
if (entry.GetCountWithDescendants() > 1) {
Assert(entry.GetTxSize() <= V3_CHILD_MAX_VSIZE);
const auto& children = entry.GetMemPoolChildrenConst();
Assert(children.begin()->get().GetTx().nVersion == 3);
}
```
to catch inheritance failures

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