🤔 maflcko reviewed a pull request: "rpc, cli: return "verificationprogress" of 1 when up to date"
(https://github.com/bitcoin/bitcoin/pull/31135#pullrequestreview-2388797020)
Not sure about the current fix. It seems to "fix" one style issue in a value that is meant as an estimate, but it may be adding two new issues at the same time?

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812815888)
Unsure, copied from `CScriptCheck`. If the state of the art of thread naming has advanced since that was written, please let me know!

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1812825825)
Seems erroneous, indeed.

💬 sipa commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812826580)
The C++ standard library does as far as I know have no way of renaming threads at all. `src/util/threadnames.{h,cpp}` is our wrapper around the various platform-dependent ways of doing so on supported systems.

💬 maflcko commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2432289339)
CI failure is unrelated, see https://github.com/bitcoin/bitcoin/issues/30969

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1812829296)
~0 on keeping it, we're reducing loc and don't really need the coverage anymore imo

Willing to be over-ridden by other reviewers.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812838378)
Can you tell me why we need to prevent multithreaded access exactly? We could collect the values to different vectors, each one accessed only by a single thread and merge them into the cache at the end on a single thread, right?

How would `mpsc` solve this better? Do you think we need work stealing to make it perfectly parallel? Wouldn't coroutines already achieve the same?

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812842053)
4x may be too much to begin with, but 1.5-2x sounds plausible, I'll help with benchmarking this once my current batches finish.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812845859)
This might be as simple as sorting by tx before we create the buckets.

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812849403)
Thank you, please resolve the comment.

📝 kevkevinpal opened a pull request: "test: added test to assert TX decode rpc error on submitpackage rpc"
(https://github.com/bitcoin/bitcoin/pull/31139)
This PR adds coverage for this line https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L996

If you run the following you will get no results for `submitpackage`
`grep -nri "TX decode failed" ./test/functional`

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812852143)
As mentioned before, why do we need shared outpoints here?

👍 instagibbs approved a pull request: "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped"
(https://github.com/bitcoin/bitcoin/pull/31040#pullrequestreview-2388836763)
ACK 6741cde5fe3575cc0b58a52c4a30058fa08c4beb

non blocking nits only

💬 instagibbs commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1812834897)
We accept new orphans, we just never exceed limits

```suggestion
self.log.info("Check that we never exceed our storage limits for orphans")
```

💬 instagibbs commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1812836992)
unsure nit: Might be faster to mine blocks until mempool is empty?

💬 instagibbs commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1812840562)
before filling up the orphanage, do we want to mock time to make it static for the duration of this test? We don't want things to be evicted even if somehow the test is slow.

💬 instagibbs commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1812848520)
if `MAX_ORPHANS` is defined it can be used here directly

💬 maflcko commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1812855024)
Yeah, this is a bit confusing. `UndefinedBehaviorSanitizer` is just the name of the tool, not an implication that the issue it found is UB.

If you want to reproduce, you'll have to compile with the `integer` sanitizer (yes, to add to the confusion, the tool is called different here). Also, it only works with `clang++`/`clang`, so you'll have to select that as well during `cmake -B`. Maybe this can be documented in `doc/developer-notes.md`?

💬 sipa commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1812858116)
I haven't yet experimented with them, but as far as I understand it, coroutines are just programming paradigm, not magic; they don't do anything of their own, besides making things that were already possible easier to write. In particular, you still need a thread pool or some mechanism for scheduling how to run them,

💬 laanwj commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1812862264)
Might want to keep this arg around for one major version, but make it display an error that the functionality has been removed. This gives a clearer error to anyone who has it in their configuration file than "argument doesn't exist".