💬 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

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

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-2651478837)
Thanks @instagibbs for the testing and review, added your fuzz commits and took comments. Still need to write the p2p_orphan_handling test.

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1951246422)
Wrote a similar test to check that an evicted work item is the one that doesn't exist in `m_orphans` anymore.

💬 arejula27 commented on pull request "fuzz: coinselection: cover `SetBumpFeeDiscount`":
(https://github.com/bitcoin/bitcoin/pull/31806#issuecomment-2651560728)
ACK [0289f03](https://github.com/bitcoin/bitcoin/pull/31806/commits/0289f03790151135afbd5281a45a9f6256f0a235)

This PR aims to improve code coverage of the fuzzing test by ensuring `SetBumpFeeDiscount` is called before the fee discount is used in `RecalculateWaste`. To achieve this, `SetBumpFeeDiscount` is added before `RecalculateWaste` is invoked. As @Prabhat1308 pointed out, there is another instance where this function could be added (line 290).

The value set by `SetBumpFeeDiscount` is
...

💬 fanquake commented on pull request "cmake: Improve safety and robustness during building `crc32c` subtree":
(https://github.com/bitcoin/bitcoin/pull/31779#issuecomment-2651620882)
Another thought, isn't this going to break git bisect, or, result in having to do unclean subtree pulls? As the commit where you update the subtree will no-longer compile unless it also simultaneously changes the hash hardcoded into our build system.

💬 achow101 commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2651711777)
ACK 0f716f28896c6edfcd4e2a2b25c88f478a029c7b