🤔 ismaelsadeeq reviewed a pull request: "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure"
(https://github.com/bitcoin/bitcoin/pull/29735#pullrequestreview-1986192988)
Concept ACK


I will run the new fuzz test with and without 72425bd699dd33d57794b3339f6266c4c9df443d

💬 ismaelsadeeq commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1555729721)
Maybe assert for this at the beginning of the `fill_mempool`
```python
assert_equal(node.getnetworkinfo()['relayfee'], Decimal('0.00001000'))
```

💬 ismaelsadeeq commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1555723093)
In c93f790cad1558dcf0b8fb5fd0cf3a276368d656 " Move fill_mempool to util function"

There should be a commit that will first pull out these two lines from this scope to
https://github.com/bitcoin/bitcoin/blob/f0794cbd405636a7f528a60f2873050b865cf7e8/test/functional/mempool_limit.py#L231

Moving `fill_mempool` to `test_framework/util.py` makes it clear that the comments do not belong to this scope.

💬 ismaelsadeeq commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1555694194)
In a466686c6e97d47e3f4718eed0f781acb88da7fa "test: expand docstring to fill_mempool"

nit:
```suggestion
Allows for simpler testing of scenarios with floating mempoolminfee > minrelayfee
```

💬 laanwj commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1555738343)
Attaching by pid is a good idea, even aside from semaphores, as it attaches a specific instance instead of all processes sharing the binary, which i think it does if you provide the path.

💬 brunoerg commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1555757479)
Also, maybe we do not need this verification anymore?
```py
if res["success"]:
self.log.info(f"Added {addr} to tx_originator's addrman")
else:
self.log.info(f"Could not add {addr} to tx_originator's addrman (collision?)")
```

💬 cbergqvist commented on pull request "test: Bump timeouts in feature_index_prune and wallet_importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/29791#issuecomment-2042641779)
Happy for my first merged PR! :)

## Post-merge post-mortem

### Working stupidly, not smart or hard

I have spent too many evenings focusing on this:
> Agree that it might be fruitful to investigate the history of wallet_importdescriptors.py to pinpoint regressions, didn't have time today.

In over >110 runs, I have observed how the chance of `wallet_importdescriptors.py --descriptors` failure has increased with time. As patterns emerged around certain commits, I've tested further only
...

💬 0xB10C commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2042651718)
I just learned that nanobench is able to fill in an [output format template](https://nanobench.ankerl.com/tutorial.html#rendering-mustache-like-templates). It might make sense to try that route first.

👍 theuni approved a pull request: "depends: add new LLVM debug macro"
(https://github.com/bitcoin/bitcoin/pull/29781#pullrequestreview-1986389340)
ACK 5efebc0edbb479d2041b3fb2d43be3a77e817b3e

💬 laanwj commented on pull request "doc: 25.2 historical release notes":
(https://github.com/bitcoin/bitcoin/pull/29830#issuecomment-2042709408)
ACK 93bd2e2f6c9672fbf1120d1e25349f6edd29cfef
Matches the file on tag `v25.2`.

💬 cbergqvist commented on pull request "p2p: gives seednode priority over dnsseed if both are provided":
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-2042723373)
ACK 2842e51a246b162a586941184b7694f187d7aee7

Diffed top 2 commits in that and 78482a09e06beb841e70781eb509c2b2fdea8bd9 which I previously tested & acked. Only scope of `start`-variable was narrowed as suggested by @davidgumberg in https://github.com/bitcoin/bitcoin/pull/28016#discussion_r1526788396.

💬 instagibbs commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1555830543)
Made it shorter :)

💬 instagibbs commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1555831622)
bit of an annoying circular reference issue(blocktools requiring utils) so for now at least make it grep-able what the magic value should be(and more greppable)

💬 instagibbs commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1555832375)
done

💬 instagibbs commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1555832564)
done, I think

💬 instagibbs commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1555832735)
done

💬 laanwj commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2042741859)
For some reason, after building with this, I dont get any output from:
```
$ gdb src/bitcoind
...
(gdb) info tracepoints
No tracepoints.
```
Tested with gdb `15.0.50.20240219-git` and `13.1`.

I checked that the tracepoints are included:
```
$ readelf -n ./src/bitcoind | grep NT_STAPSDT -A 4 -B 2
stapsdt 0x00000062 NT_STAPSDT (SystemTap probe descriptors)
Provider: net
Name: outbound_message
Location: 0x00000000001809bc, Base: 0x0000000000a2cb60, S
...

💬 stickies-v commented on pull request "clang-tidy: Enable misc-no-recursion":
(https://github.com/bitcoin/bitcoin/pull/29690#issuecomment-2042758379)
ACK 78407b99ed6dd17f687fcbfb0486ecc433302287

👍 ismaelsadeeq approved a pull request: "doc: 25.2 historical release notes"
(https://github.com/bitcoin/bitcoin/pull/29830#pullrequestreview-1986526306)
ACK 93bd2e2f6c9672fbf1120d1e25349f6edd29cfef

💬 laanwj commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2042827943)
Oh, it's `info probes` not `info tracepoints`. Whoops. It checks out now.

Code review and lightly tested ACK 3933bdd6cfc9accab9e0ed47e83a5cc27ada6b68

💬 laanwj commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1555936795)
It's slightly inconsistent to pass this parameter as int, while the other enum (Connection Type) gets passed as string. Might similarly pass `GetNetworkName(...).c_str()`. Especially after #26593 which would remove the call overhead in the happy path.
No strong opinion though, I think the integer value is easier to use if you're going to make a histogram or such.