💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-1854284485)
Rebased. Now includes the tracepoint changes from https://github.com/bitcoin/bitcoin/pull/25273.

💬 theStack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1854344432)
> I guess in general it's less nice to add options to `-cli` args compared to rpcs: The alternative is that we could allow some construction like `-connect=1.2.3.4:8333|v1transport` (and similar to `-addnode` / `-seednode`) but then we need additional string parsing / documentation for that, and I think we should only do it if there is an actual use case for it.

I agree, this should only be done if there is a really good reason for it.

@naumenkogs: Note that my concerns were not about the
...

💬 brunoerg commented on pull request "fuzz: coinselection, improve `min_viable_change`/`change_output_size`":
(https://github.com/bitcoin/bitcoin/pull/28372#issuecomment-1854350212)
CI error seems unrelated:
```
2023-12-13T15:26:02.5983445Z 54/286 - rpc_signer.py failed, Duration: 3 s
2023-12-13T15:26:02.5983915Z
2023-12-13T15:26:02.5984128Z stdout:
2023-12-13T15:26:02.5985467Z 2023-12-13T15:25:59.249000Z TestFramework (INFO): PRNG seed is: 6112864330657271854
2023-12-13T15:26:02.5986266Z
2023-12-13T15:26:02.5994148Z 2023-12-13T15:25:59.249000Z TestFramework (INFO): Initializing test directory D:\a\_temp\test_runner_₿_🏃_20231213_151925\rpc_signer_229
2023-12-13T1
...

👍 stickies-v approved a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1780105857)
re-ACK 6db04be102807ee0120981a9b8de62a55439dabb

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