Bitcoin Core Github
44 subscribers
120K links
Download Telegram
💬 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
💬 achow101 commented on issue "Wallet symlinks are not rejected as expected":
(https://github.com/bitcoin/bitcoin/issues/31842#issuecomment-2651774811)
Symlink to the wallet directory is allowed, but a symlink to a wallet file is not. What you've tested manually is a symlink to a wallet directory, so it works.
🚀 achow101 merged a pull request: "test: Add mockable steady clock, tests for PCP and NATPMP implementations"
(https://github.com/bitcoin/bitcoin/pull/31022)
🤔 mzumsande reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2607117435)
Halfway through, some minor thing below - my main conceptual question is why `m_total_announcements` is a meaningful metric in limiting the orphanage.

My understanding is that `m_total_orphan_usage` exists to limit memory usage, and `m_total_announcements` to limit CPU usage - but why the number of announcements instead of number of orphans?
Why would it make the situation any less DoSy if we remove an announcer but keep the orphan? Since we only assign the tx to one peer's workset after 74
...