💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2299384096)
re: https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2299186191

> What is the criteria for including a parameter in this table?

Basically if any string parameter for a method can contain `=`, all the other string parameters for the method should be listed too. My cleanup of this code in b998cc52d51b48db9271fdba0bd69e9aaccb7999 adds a comment explaining this:

https://github.com/bitcoin/bitcoin/blob/b998cc52d51b48db9271fdba0bd69e9aaccb7999/src/rpc/client.cpp#L36-L42

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2299391976)
> In [a4f2144](https://github.com/bitcoin/bitcoin/commit/a4f2144facb5d4dc2f749494ea65744fffcb628b) "rpc: Handle -named argument parsing where '=' character is used"
>
> Instead of parsing the argument directly, we should be using either `Parse` or `ArgToUniValue`

I don't think you can use `Parse` or `ArgToUniValue` unless you catch the exception they throw. No strong opinion, but I think it is probably simpler to use read.

💬 l0rinc commented on pull request "Don't zero-after-free `DataStream`: Faster IBD on some configurations":
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-3222203456)
@davidgumberg are you still working on this? I want to investigate the same problem, it may still be relevant for Raspberry Pis.

⚠️ pirsonxyz opened an issue: "rewrite in rust"
(https://github.com/bitcoin/bitcoin/issues/33255)
### Please describe the feature you'd like to see added.

Please, i want a fast network

### Is your feature related to a problem, if so please describe it.

_No response_

### Describe the solution you'd like

_No response_

### Describe any alternatives you've considered

_No response_

### Please leave any additional context

_No response_

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