💬 fjahr commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r1951154411)
Yes, this would be simpler but I am not sure if we want it to be simpler. With fuzzing we usually try to hit as different many scenarios as possible and that's why I chose this, sometimes setting `block_reserved_weight` and sometimes not, and setting it to a variety of values. I'm not an expert on fuzzing though, maybe someone with a focus on it can weight in here.

💬 sr-gi commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#issuecomment-2651316273)
The last commit had a small typo, force-pushing to fix it

🤔 BrandonOdiwuor reviewed a pull request: "test: check `scanning` field from `getwalletinfo`"
(https://github.com/bitcoin/bitcoin/pull/31768#pullrequestreview-2609296885)
Code Review ACK bb0879ddabc8b3a7253bc774d23b842937d18015

👍 theuni approved a pull request: "guix: remove test-security/symbol-check scripts"
(https://github.com/bitcoin/bitcoin/pull/31818#pullrequestreview-2609339794)
utACK 76c090145e9bb64fe4ef6a663723dd0e9028ed10

💬 maflcko commented on pull request "test: Clear scheduler after init to increase fuzz stability":
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2651420109)
Actually, this is incomplete. It will handle the coverage generated in the notification itself (`f()`), but there is no guarantee that the re-lock is handled as well when the context ends happens before fuzz target execution starts after init. Relevant scope for the re-lock:

https://github.com/bitcoin/bitcoin/blob/86528937e5c4da2e12c46085fc41e87ed759258e/src/scheduler.cpp#L56-L61

The change here should be an improvement, but I wonder if there is a trivial one-line complete solution.

On
...

💬 furszy commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1951218506)
> Rather than making the RPC output field optional, can we not fallback to always returning a `raw` / `addr` descriptor if inferring returns something that would be invalid?

Hmm, I would prefer to avoid returning `addr()` descriptors when the script cannot be redeemed due to consensus rules. Mainly because it opens up the remote possibility of users sending money to it (since the address is there).

This isn't an issue in this `gettxout()` case because the script comes from an unspent outpu
...

💬 marcofleon commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2651426767)
Is there a disadvantage to running the harness twice using `-runs=1` or even more and then comparing the coverage reports? Vs going through the inputs one by one.

Separately (but maybe related?), I ran this script again on `p2p_headers_presync` and there was no failure. If you look at these two coverage reports and go to line 1126, you'll see the different hit counts:

https://marcofleon.github.io/coverage/p2p_headers_presync/coverage/root/bitcoin/src/net_processing.cpp.html
https://marcof
...

🤔 mzumsande reviewed a pull request: "streams: Add stream position validation in BufferedFile::AdvanceStream"
(https://github.com/bitcoin/bitcoin/pull/31839#pullrequestreview-2609402837)
> The prior assert with the exact same check covers this. This is adding dead code.

`Fill()` may be called in between, which can change `nSrcPos` - but it can only increase it, so yes, still dead code.

💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2651462016)
> Is there a disadvantage to running the harness twice using `-runs=1` or even more and then comparing the coverage reports? Vs going through the inputs one by one.

I am thinking that sometimes the non-determinism is specific to one input. For example, if a piece of code is completely unreachable by one input, it will never be non-deterministic, so running over that seed to reproduce the non-determinism is just busy-work.

> edit: I guess if the target is unstable then maybe it wouldn't fai
...

💬 marcofleon commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#discussion_r1951234145)
Accidental parentheses at the end here?

💬 theuni commented on pull request "cmake: Install man pages for configured targets only":
(https://github.com/bitcoin/bitcoin/pull/31765#issuecomment-2651471391)
Considering that in order to hook up `COMPONENTS` usage, we'd need to add an `install()` line for each as opposed to our current `installable_targets` approach, I'm not sure that `install_manpage` is the right abstraction.

Instead, I think we're going to want an `add_install()` or so, right?

For ex:

Instead of:
```CMake
list(APPEND installable_targets bitcoind)
...
install(TARGETS ${installable_targets}
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
)
```

We'll want:
```CMake
...

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1950722422)
thanks, fixed

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1950725244)
added

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1950722979)
removed

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1950778587)
added

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1950789783)
Wow, very bad bit flip :facepalm: thank you

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1950745977)
Yes, and they are each announced by every peer. This bench is to test the maximum number of transactions where every peer has 100% overlap. We call `AddTx` `DEFAULT_MAX_ORPHAN_ANNOUNCEMENTS` times, which is the maximum before eviction would trigger. If we increase `DEFAULT_MAX_ORPHAN_ANNOUNCEMENTS`, the bench will scale too.

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1950724798)
changed

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1950780433)
done

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1950739715)
Added