💬 kevkevinpal commented on pull request "test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)":
(https://github.com/bitcoin/bitcoin/pull/33023#discussion_r2299239940)
what is the point of testing with 2 when you have another test, testing with 1?

💬 kevkevinpal commented on pull request "test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)":
(https://github.com/bitcoin/bitcoin/pull/33023#discussion_r2299227722)
This seems misplaced for this test `test_extratxn_invalid_parameters`

💬 kevkevinpal commented on pull request "test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)":
(https://github.com/bitcoin/bitcoin/pull/33023#discussion_r2299245392)
If you could why not set buffersize to 400, and then after checking the capacity test the wrap around?

Then you can drop `test_extratxn_large_capacity` and `test_extratxn_buffer_wraparound` seems redundant to do the setup multiple times when they can be done in the same test.

💬 l0rinc commented on issue "bitcoind 29.0 much slower than 28.0 on my system: cause found":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-3221920491)
I started one at the same time with some cpu and lock profiling and just stopped it today to see the results. I will see if https://github.com/bitcoin/bitcoin/issues/32832 still reproduces, it's really surprising how slow this is on a Pi.

💬 polespinasa commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2299271743)
I think this should be:
```c++
if (txn_available[idit->second] &&
txn_available[idit->second]->GetWitnessHash() != extra_txn[i].first) { ...
```

🤔 polespinasa reviewed a pull request: "Revert compact block cache inefficiencies"
(https://github.com/bitcoin/bitcoin/pull/33253#pullrequestreview-3153218666)
Concept ACK

Nice catch
No reversions eb1920a82ffa2086c3e8e3e8946b9f43053455aa:

| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 2,163,341.00 | 462.25 | 7.4% | 0.03 | `BlockEncoding`


Only f2b9cbed7196617880d177c089332e4fd48f28ca reverted

| ns/op | op/s | err% | total | benchmark
|--------------------:|------
...

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2299342031)
I find it almost a philosophical question if we should cover reorganizing the BIP30 blocks so I honestly didn't really think much about this and I added it almost by accident when reorganizing (is that a pun?) all the commits between the changes here and #33134 but I guess the removal of the checkpoints and surrounding discussions recently pushed me more into the camp of doing this fully correctly and this is why I added it when I previously didn't. I will move this into it's own commit.

> Th
...

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3222023488)
First of all, sorry for the late reply, I will do my best to respond faster despite travelling at the moment.

> [053ac55](https://github.com/bitcoin/bitcoin/commit/053ac55eb569be6b49c5249a7d8c0eeaa149a18b) seems like it could cause incorrect total statistics if somehow we ever have the situation where we are appending a block that doesn't build on the index's current block hash.
>
> This behavior does match what we do today with the running total members, but it feels incorrect. Although t
...

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3222024490)
Addressed the feedback and also rebased for good measure since there were recent changes to the indexing.

💬 bigshiny90 commented on pull request "test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)":
(https://github.com/bitcoin/bitcoin/pull/33023#discussion_r2299367679)
removed. now just tests for invalid capacity -1

💬 bigshiny90 commented on pull request "test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)":
(https://github.com/bitcoin/bitcoin/pull/33023#discussion_r2299369204)
done

💬 bigshiny90 commented on pull request "test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)":
(https://github.com/bitcoin/bitcoin/pull/33023#discussion_r2299369564)
yes, removed redundant test

💬 bigshiny90 commented on pull request "test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)":
(https://github.com/bitcoin/bitcoin/pull/33023#discussion_r2299372290)
I combined the 3 separate tests - test_extratxnpool_capacity, test_extratxn_large_capacity, test_extratxn_buffer_wraparound - and created one test test_extratxnpool_capacity_and_wraparound, which tests capacity 400 and wraparound behavior

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

> I find the name 'next_...` to be confusing since this is really referring to the position of the current argument, not the next argument.

Technically these aren't really referring to the current argument `s`. At this point in the loop, all we know about `s` is that it contains an `=` sign and that the beginning part of `s` before `=` is not a known parameter name. So `s` might be passed by-position if it is compatib
...

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