👍 fanquake approved a pull request: "msvc: Fix `test\config.ini` content"
(https://github.com/bitcoin/bitcoin/pull/29075#pullrequestreview-1780042081)
ACK f76e59d02ea819928b45bd18d9c3a5b1aab36fe2

🚀 fanquake merged a pull request: "msvc: Fix `test\config.ini` content"
(https://github.com/bitcoin/bitcoin/pull/29075)

✅ fanquake closed an issue: "ci: MSVC failures"
(https://github.com/bitcoin/bitcoin/issues/29074)

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1425606237)
this only works if prioritization happens before mempool acceptance

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