🤔 kannapoix reviewed a pull request: "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py"
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3150220430)
I'm new to this code base, so I might have missed something or made some mistakes in my review.
Please feel free to correct me if that's the case. Thanks!

💬 kannapoix commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2297317597)
Is there a reason we need to explicitly specify fee_rate here?
From what I can see, the test seems to pass even without setting this value.
I understand that in the original test, the transaction with the OP_RETURN was sent with a fee rate of 0.003 implicitly. However, since we are not testing the fee rate in this case, I'm concerned that explicitly setting it here might be confusing for future readers.

💬 kannapoix commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2297380324)
If we are not specifically testing address type or scripts here, perhaps we could use `wallet.send_self_transfer(from_node=self.nodes)` instead?
I think this might make the code simpler than generating scripts and using wallet.send_to.

💬 kannapoix commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2299427937)
May I ask why we need to mine here after the first transaction?
I noticed that the original test also mines at this point, but I’m not sure about the reason behind it:
https://github.com/bitcoin/bitcoin/blob/6ca6f3b37b992591726bd13b494369bee3bd6468/test/functional/rpc_getblockstats.py#L53-L54

💬 kannapoix commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2299494594)
Just to confirm, it seems we need to re-serialize the transaction after changing the output.
Is that correct?

```suggestion
wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=tx["tx"].serialize().hex())
```

✅ davidgumberg closed a pull request: "Don't zero-after-free `DataStream`: Faster IBD on some configurations"
(https://github.com/bitcoin/bitcoin/pull/30987)

💬 davidgumberg commented on pull request "Don't zero-after-free `DataStream`: Faster IBD on some configurations":
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-3222435332)
I was intending to get back to this eventually but I'm not working on this at the moment, feel free to consider anything in this PR up for grabs.

💬 kannapoix commented on issue "`rpc_getblockstats.py` fails with `--gen-test-data`":
(https://github.com/bitcoin/bitcoin/issues/31838#issuecomment-3222438327)
For anyone reproducing this issue at the current master HEAD: Please add the flag `-deprecatedrpc=settxfee`.

✅ achow101 closed an issue: "rewrite in rust"
(https://github.com/bitcoin/bitcoin/issues/33255)

💬 w0xlt commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#issuecomment-3222458339)
@l0rinc thank you for your detailed reviews.

> I personally would not try to correct the behavior (especially silently), given that it's rare, I would simply give the user a warning or error and let them fix the arguments.

I adjusted the approach to meet this requirement. Now when duplicates are detected, the node terminates with a clear, specific error message: "_Duplicate binding configuration for address < addr >. Please check your -bind, -whitebind, and onion binding settings._"

I d
...

💬 kannapoix commented on issue "`rpc_getblockstats.py` fails with `--gen-test-data`":
(https://github.com/bitcoin/bitcoin/issues/31838#issuecomment-3222459293)
For anyone reproducing this issue at the current master HEAD: Please add the flag -deprecatedrpc=settxfee.

```diff
diff --git a/test/functional/rpc_getblockstats.py b/test/functional/rpc_getblockstats.py
index 002763201a5..a904e56d7f0 100755
--- a/test/functional/rpc_getblockstats.py
+++ b/test/functional/rpc_getblockstats.py
@@ -24,6 +24,7 @@ class GetblockstatsTest(BitcoinTestFramework):
max_stat_pos = 2

def add_options(self, parser):
+ self.add_wallet_options(parser)

...

💬 ajtowns commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2299685034)
This is just testing how quickly we can iterate through all the txs in the mempool when looking at a compact block -- which is the worst case scenario that can still be fast (if we finish solving the block with transactions from the extra pool, and don't need to go back to the network). Hitting duplicates automatically puts us in the slow resolve-via-network path, so benching that shouldn't be terribly interesting.

💬 ajtowns commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2299690928)
Once we've found a shortid match, looking up the actual tx is fine, so I don't think this warrants changing.

💬 ajtowns commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3222540332)
> Can we bench that? Maybe as a separate benchmark instead of having this one benchmark include both.

Added benchmarks with 50k txs in mempool and 0, 100 (default), and 5000 entries in extra pool. For reference [datum](https://github.com/OCEAN-xyz/datum_gateway) recommends setting the value to 1M.

💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2299706227)
To make sure that's what we're testing, we should still assert the outcome of `InitData` was `READ_STATUS_OK` - otherwise it would be possible that we accidentally generated duplicates here.

💬 w0xlt commented on issue "`feature_bind_extra.py` test fails in `test_runner` if new nodes are added":
(https://github.com/bitcoin/bitcoin/issues/33250#issuecomment-3222620994)
But MAX_NODES has a value of 12.
In the above patch, the number of nodes has been increased to 6.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2299891671)
The definition might be the same, but the purpose is entirely different. This is specifically for string RPC methods.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2299891945)
There was a `@note` at around lines 328-330 explaining this behaviour.

💬 marcofleon commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3223004584)
> we should mostly fuzz to gain confidence in stability and correctness, for which I'd suggest:

I'm happy to work on this. Still thinking about the best approach. It seems like using fuzzamoto is ultimately most effective, but it might be worth trying to get a baseline single-process fuzz test into the repo? This would probably be a more practical starting point for me too, but I'm open to suggestions.

💬 Sjors commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3223044161)
> a baseline single-process fuzz test into the repo?

I think so yes, otherwise it might not make the branch-off.