💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2299170764)
In a4f2144facb5d4dc2f749494ea65744fffcb628b "rpc: Handle -named argument parsing where '=' character is used"

This seems unnecessary, it's the same thing `CRPCConvertParam`.

💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2299177873)
In a4f2144facb5d4dc2f749494ea65744fffcb628b "rpc: Handle -named argument parsing where '=' character is used"

Instead of parsing the argument directly, we should be using either `Parse` or `ArgToUniValue`

💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2299171197)
In a4f2144facb5d4dc2f749494ea65744fffcb628b "rpc: Handle -named argument parsing where '=' character is used"

nit: these should use the new naming style.

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

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

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

What is the criteria for including a parameter in this table? This text says "parameters that might contain '=' character", but this includes things that should never have `=` in them, like sighashtype, estimate_mode, or blockhash. But if the inclusion criteria is basically any string a user can provide, then there are several string parameters that should be included, like all of the
...

🤔 kevkevinpal reviewed a pull request: "test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)"
(https://github.com/bitcoin/bitcoin/pull/33023#pullrequestreview-3153147837)
I get the idea of testing the extra pool. I think some of the tests can be consolidated.

I did run the test locally, and it passes for me. I haven't checked code coverage either to see if it covers anything extra,

but `grep -nri "blockreconstructionextratxn" ./test/functional/` does not have any matches

💬 kevkevinpal commented on pull request "test: Add functional tests for blockreconstructionextratxn and extra pool (compactblocks)":
(https://github.com/bitcoin/bitcoin/pull/33023#discussion_r2299231394)
instead of 8 can you make this `num_txs - count`, it makes sense to not have magic numbers

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