💬 rkrux commented on pull request "test: fix accurate multisig sigop count (BIP16), add unit test":
(https://github.com/bitcoin/bitcoin/pull/29615#discussion_r1555562510)
> Note that this was unnoticed, as the code part was never hit so far in the test framework.

The other occurrences where `true` is passed end up being ultimately called by `feature_block.py` test via `get_legacy_sigopcount_block` and `get_legacy_sigopcount_tx` functions. Am I missing something here?

🤔 nothingmuch reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-1984991157)
cACK, with a suggestion and some observations on the design space WRT reliability/success.

> Yes, if `-proxyrandomize=1` (the default). This is the same in `master` - separate Tor circuit per connection.

If this is not the case and a peer to which a long lived connection already exists is selected again, then tor will multiplex both connections on the same established circuit, which means that the two connections are unambiguously correlated.

Avoiding already connected peers would work
...

💬 nothingmuch commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1554905733)
With regards to this comment and:

> We pick a random Tor (or I2P) peer.

To increase success rate and reduce latency, clearnet peers could be also be selected when using Tor, without harming privacy of broadcast...

Clearnet connections only need one Tor circuit, whereas hidden service connections which require two circuits for the connecting peer and one from the accepting peer.

Furthermore, if SOCKS proxy credentials are reused for the broadcast of a single transaction (but not all t
...

💬 0xB10C commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2042391197)
> If using https://codespeed.bitcoinperf.com doesn't work out, I have created a continuous benchmarking for doing exactly this, Bencher: https://github.com/bencherdev/bencher
>
> Bencher tracks changes over time. It can easily be run in CI as a GitHub Action, and it has statistical thresholds to detect deviations.

For Bitcoin Core, it would be useful to have an adapter for the nanobench JSON output. To track this, I've opened https://github.com/bencherdev/bencher/issues/361.

🤔 rkrux reviewed a pull request: "tests: fix `OP_1NEGATE` handling in `CScriptOp`"
(https://github.com/bitcoin/bitcoin/pull/29589#pullrequestreview-1986101030)
1. Make is successful
2. All functional tests pass

Would it be possible to share where this error pops up in the branch specified in the description?
> "When we don't encode -1 as OP_1NEGATE then we end up getting errors inside of CheckMinimalPush() saying -1 was not minimally encoded."

👍 dergoegge approved a pull request: "refactor: Use typesafe Wtxid in compact block encodings"
(https://github.com/bitcoin/bitcoin/pull/29752#pullrequestreview-1986159858)
ACK a8203e94123b6ea6e4f4a6320e3ad20457f44a28

💬 sr-gi commented on pull request "test: Cleans some manual checks/drops when using wait_for_getdata":
(https://github.com/bitcoin/bitcoin/pull/29748#issuecomment-2042562089)
> Concept ACK, nit: Could you also take a look at `announce_tx_and_wait_for_getdata` in `p2p_segwit.py` for a similar cleanup?

Unfortunately, that's not the case given how the tests using `announce_tx_and_wait_for_getdata` are built. That is, the same transaction may be sent multiple times using `announce_tx_and_wait_for_getdata`, so you need to potentially pop the old one to make sure the assert is not using old data.

The only way of working around this would be making `wait_for_getdata`
...

💬 sipa commented on pull request "Enable HW-accelerated implementations of SHA256 for MSVC builds":
(https://github.com/bitcoin/bitcoin/pull/24773#discussion_r1555701766)
@theuni I think what is going on is that before this PR, `HAVE_GETCPUID` wasn't defined for MSVC builds, but with this PR, there now is such a feature. However, this PR still doesn't enable `rdrand`/`rdseed` support for Windows (it would also need intrinsics versions), so this define conditional is needed to exclude support for that despite having getcpuid.

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

Please feel free to ignore nits

💬 murchandamus commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1555720689)
Nit: I don’t feel strongly about it, but the change that made me notice that there were code changes in the move originally was that `COINBASE_MATURITY - 1` had gotten replaced with a `99` magic number.

💬 murchandamus commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1555727656)
Nit: Subject line in commit message should not exceed 50 characters, and definitely not be more than 72: https://cbea.ms/git-commit/#limit-50

🤔 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