👍 vasild approved a pull request: "i2p: fix and improve logs"
(https://github.com/bitcoin/bitcoin/pull/29833#pullrequestreview-2049903727)
ACK 469e4834bad295f6d9fbf8048a8db23aa758bac9

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596649444)
Split up the commit, hope it helps (https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2049866191)

💬 cbergqvist commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1596649820)
Adding 5 to the timeout fixes it for me. Modifying the line with the mocktime does not (I also tried removing that line which caused the test to fail again, proving its usefulness).
```
with node.assert_debug_log(expected_msgs=[f"Couldn't connect to peers from addrman after {ADD_NEXT_SEEDNODE} seconds. Adding seednode ({seed_node}) to addrfetch"], unexpected_msgs=["Empty addrman, adding seednode"], timeout=ADD_NEXT_SEEDNODE + 5):
self.restart_node(0, extra_args=[f'-seednod
...

🤔 ismaelsadeeq reviewed a pull request: "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`"
(https://github.com/bitcoin/bitcoin/pull/29939#pullrequestreview-2049928550)
Post merge Tested ACK dd8fa861939d5b8bdd844ad7cab015d08533a91a

💬 rkrux commented on pull request "test: fix MiniWallet script-path spend (missing parity bit in leaf version)":
(https://github.com/bitcoin/bitcoin/pull/30076#discussion_r1596687155)
I feel we should tie this `only-path` here to `taproot_construct` call in `address.py`, either by passing it as a parameter or creating it as a constant. Otherwise, it makes you wonder here at this line where did `only-path` come from.

👍 rkrux approved a pull request: "test: fix MiniWallet script-path spend (missing parity bit in leaf version)"
(https://github.com/bitcoin/bitcoin/pull/30076#pullrequestreview-2049966561)
tACK [e4abac2](https://github.com/bitcoin/bitcoin/pull/30076/commits/e4abac2fce4af98108d899bc0480d066e5da7754)

Reviewing #29939 previously made it easy for me to review this one!

Make successful and all functional tests pass on this branch. Using the shared patch, I can see these 2 tests fails in master branch.

```
feature_versionbits_warning.py | ✖ Failed | 3 s
wallet_fundrawtransaction.py --descriptors | ✖ Failed | 17 s

ALL
...

💬 rkrux commented on pull request "test: fix MiniWallet script-path spend (missing parity bit in leaf version)":
(https://github.com/bitcoin/bitcoin/pull/30076#discussion_r1596687535)
Would love to see the corresponding cpp reference code, share if you know?

💬 josibake commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1596696710)
Awesome, thanks @theuni ! I pulled this commit in and also add a "simple read only vector like interface" to the `KeyPair` class (same as `CKey`). Needed for: https://github.com/bitcoin/bitcoin/commit/5af5164b84b534c9a3171d898c09125957b21ba1#diff-e5cbd40011e760f175b82d7ec44ad512bdb3d9108068b660642204da9c9187b1R204

I also rebased #28122 on this PR and confirmed everything works with the new `KeyPair` class.

🤔 glozow reviewed a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2049995289)
reACK 6a39183b53

💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1596704440)
```suggestion
// Don't attempt to process the same conflicts repeatedly during subpackage evaluation:
```

💬 josibake commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2104541750)
Updated to use the new `KeyPair` wrapper class (h/t @theuni).

💬 TheCharlatan commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#issuecomment-2104546249)
Guix build (aarch64):
```
cf21ea004d1f8d6d7b53d4ef725083229b18469948439f79872a2b019e8bc966 guix-build-7efd6bf03755/output/aarch64-linux-gnu/SHA256SUMS.part
1fa79040aba310b34075b11eefbdb570dd1f053d25ea432faf9bca596603cabb guix-build-7efd6bf03755/output/aarch64-linux-gnu/bitcoin-7efd6bf03755-aarch64-linux-gnu-debug.tar.gz
a97ee58be3ea463c565ec8098ed3f98a04f70b7a897c2b1961df083f5142e07b guix-build-7efd6bf03755/output/aarch64-linux-gnu/bitcoin-7efd6bf03755-aarch64-linux-gnu.tar.gz
1f4f554289
...

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596668853)
nit: replace "include the trace.h" with either "include trace.h" or "include the trace.h header"

🤔 vasild reviewed a pull request: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593#pullrequestreview-2049935876)
Approach ACK 38e24dfc5f1c54723018bf9d973b7d9cf9a2b824

Mostly nits that would be nice to address + one blocker about the `bitcoind_pid` vs `pid` mismatch.

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596671187)
nit:
```suggestion
// Setting SDT_USE_VARIADIC lets systemtap (sys/sdt.h) know that we want to use
```

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596685441)
Indentation in Bitcoin Core is 4 spaces. This file uses 2 spaces (also some of the examples in `doc/tracing.md`).

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596683957)
nit:
```suggestion
if (TRACEPOINT_ACTIVE(example, gated_expensive_argument)) {
```

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596676716)
3 nits:

```suggestion
If needed, an extra `if (TRACEPOINT_ACTIVE(context, event)) {...}` check can be
```

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596711392)
I know it would be weird usage if somebody does something like `TRACEPOINT_ACTIVE(...) + 1` but in general it is better to define macros in such a way to avoid unexpected outcomes. In that case it would translate to `context##_##event##_semaphore > (0 + 1)` whereas the expectation would be `(context##_##event##_semaphore > 0) + 1`.

```suggestion
#define TRACEPOINT_ACTIVE(context, event) (context##_##event##_semaphore > 0)
```

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596714759)
whitespace:
```suggestion
if (TRACEPOINT_ACTIVE(context, event)) { \
STAP_PROBEV(context, event __VA_OPT__(, ) __VA_ARGS__); \
} \
```