💬 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
(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.
(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.
(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
...
(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.
(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
(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.
(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.
💬 0xB10C commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2651790997)
here a some per task stats on this:
e.g. for the `fuzzer,address,undefined,integer, no depends` task
scheduling + execution:
https://0xb10c.github.io/bitcoin-core-ci-stats/graph/schedule-and-execution/
scheduling only:
https://0xb10c.github.io/bitcoin-core-ci-stats/graph/schedule/
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2651790997)
here a some per task stats on this:
e.g. for the `fuzzer,address,undefined,integer, no depends` task
scheduling + execution:
https://0xb10c.github.io/bitcoin-core-ci-stats/graph/schedule-and-execution/
scheduling only:
https://0xb10c.github.io/bitcoin-core-ci-stats/graph/schedule/
🚀 achow101 merged a pull request: "test: Add mockable steady clock, tests for PCP and NATPMP implementations"
(https://github.com/bitcoin/bitcoin/pull/31022)
(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
...
(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
...
💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949998393)
should call `reserve` before pushing entries to `peer_it_heap`, not after.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949998393)
should call `reserve` before pushing entries to `peer_it_heap`, not after.
💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949955925)
should this be `int64_t` instead of `int` in the spirit of the first commit?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949955925)
should this be `int64_t` instead of `int` in the spirit of the first commit?
💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1950002603)
Could `m_orphans.size() <= max_orphans` be inside `NeedsTrim`?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1950002603)
Could `m_orphans.size() <= max_orphans` be inside `NeedsTrim`?
💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949856455)
nit: doesn't really matter if unsigned int (return value) or int is used, but would be nice to make it consistent, also with the %u / %d format specifiers in the logprints.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949856455)
nit: doesn't really matter if unsigned int (return value) or int is used, but would be nice to make it consistent, also with the %u / %d format specifiers in the logprints.
💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949899180)
nit: commit msg of c929d71c0828544be509934312b6a7d11b47ea4d lacks a verb (e.g. "split").
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949899180)
nit: commit msg of c929d71c0828544be509934312b6a7d11b47ea4d lacks a verb (e.g. "split").
💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2651900348)
re: https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2546088852
> I found another segmentation fault in updating my previous ACK to the latest tip of this PR. Here are minimal steps to reproduce
This seems closely related to the _broken connection during node shutdown causing unclean shutdown_ issue reported earlier so added a note about this there: https://github.com/chaincodelabs/libmultiprocess/issues/123#issuecomment-2651851492
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2651900348)
re: https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2546088852
> I found another segmentation fault in updating my previous ACK to the latest tip of this PR. Here are minimal steps to reproduce
This seems closely related to the _broken connection during node shutdown causing unclean shutdown_ issue reported earlier so added a note about this there: https://github.com/chaincodelabs/libmultiprocess/issues/123#issuecomment-2651851492
💬 maflcko commented on pull request "fuzz: Use serial task runner to increase fuzz stability":
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2651908196)
> On alternative would be to inline the scheduler in tests (make it synchronous), but the patch would be larger.
Looks like this will lead to deadlocks due to lock order inversions between cs_main and cs_wallet in the unit tests. I'll try to apply it to the fuzz tests only. If there are still issues in this pull, or in the future, they could be worked around for the affected fuzz target.
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2651908196)
> On alternative would be to inline the scheduler in tests (make it synchronous), but the patch would be larger.
Looks like this will lead to deadlocks due to lock order inversions between cs_main and cs_wallet in the unit tests. I'll try to apply it to the fuzz tests only. If there are still issues in this pull, or in the future, they could be worked around for the affected fuzz target.
💬 Prabhat1308 commented on pull request "test: clarify timewarp grace period griefing attack":
(https://github.com/bitcoin/bitcoin/pull/31725#discussion_r1951495427)
this is introducing a more stricter equality. Is it because we now have the liberty to choose `future` as we wish whereas previously it was `t + MAX_FUTURE_BLOCK_TIME` which not necessarily be equal to template's curtime ?
(https://github.com/bitcoin/bitcoin/pull/31725#discussion_r1951495427)
this is introducing a more stricter equality. Is it because we now have the liberty to choose `future` as we wish whereas previously it was `t + MAX_FUTURE_BLOCK_TIME` which not necessarily be equal to template's curtime ?
💬 Prabhat1308 commented on pull request "test: clarify timewarp grace period griefing attack":
(https://github.com/bitcoin/bitcoin/pull/31725#discussion_r1951488861)
The log mentions `t` to be strictly less than `MAX_FUTURE_BLOCK_TIME`. But in line 165 , the assert statement is for `future<=MAX_FUTURE_BLOCK_TIME`.
(https://github.com/bitcoin/bitcoin/pull/31725#discussion_r1951488861)
The log mentions `t` to be strictly less than `MAX_FUTURE_BLOCK_TIME`. But in line 165 , the assert statement is for `future<=MAX_FUTURE_BLOCK_TIME`.
💬 Prabhat1308 commented on pull request "test: clarify timewarp grace period griefing attack":
(https://github.com/bitcoin/bitcoin/pull/31725#discussion_r1951491551)
on previous line 199 , `mtp` is set to `node.getblock(node.getbestblockhash())["mediantime"] + 1` . from the rpc commands , shouldnt `mtp` be just `node.getblock(node.getbestblockhash())["mediantime"]` ? and then `nTime` is correct with `mtp + 1`. Have checked it locally and the test passes.
(https://github.com/bitcoin/bitcoin/pull/31725#discussion_r1951491551)
on previous line 199 , `mtp` is set to `node.getblock(node.getbestblockhash())["mediantime"] + 1` . from the rpc commands , shouldnt `mtp` be just `node.getblock(node.getbestblockhash())["mediantime"]` ? and then `nTime` is correct with `mtp + 1`. Have checked it locally and the test passes.