💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298827191)
nit: direct iteration would be slightly simpler:
```suggestion
for (auto& tx : refs) {
AddTx(tx, /*fee=*/tx->vout[0].nValue, pool);
}
```

💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298808174)
nit: `std::ranges::shuffle` might be simpler:
```suggestion
std::ranges::shuffle(refs, rng);
```

💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298810753)
we could consume the result to make sure the benchmark isn't optimized away

💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298820059)
alternatively this may be slightly more descriptive than `11` and it could even be used to init the sigspam:
```suggestion
FastRandomContext rng{/*fDeterministic=*/true};
auto sigspam{rng.randbytes<200>()};
```

💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298807140)
nit: we might as well reserve `50'000` here

💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298886174)
I redid the a8203e94123b6ea6e4f4a6320e3ad20457f44a28 cherry-pick, this line and the top comment are new additions (+ some conflicts were resolved slightly differently to how I have, but the result seems correct to me)

💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298873346)
nit:
```suggestion
BenchCBHAST cmpctblock{rng, /*txs=*/3'000};
```

💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2299068765)
as mentioned by @instagibbs and @achow101, we could add two benches here, for empty extra and with some (partially overlapping) one, something like:
```C++
std::vector<std::pair<Wtxid, CTransactionRef>> extra_txn;
// ...
extra_txn.reserve(extra_tx_count);
for (size_t i{0}; i < extra_tx_count; ++i) {
size_t index{i < (extra_tx_count / 2) ? mempool_size + i : mempool_size - i}; // some overlap with mempool
auto tx{CreateTestTransaction(sigspam
...

💬 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) { ...
```