💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949807125)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949807125)
Fixed.
💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2649120502)
@instagibbs I incorporated a variant of your commit into the existing `feefrac_mul_div` fuzz test, with the tightest bounds I could make more.
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2649120502)
@instagibbs I incorporated a variant of your commit into the existing `feefrac_mul_div` fuzz test, with the tightest bounds I could make more.
💬 instagibbs commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949815030)
I think there's also some value in knowing we're not diverging wildly from what already exists if we're planning on swapping functionality later. Agreed on the rest.
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949815030)
I think there's also some value in knowing we're not diverging wildly from what already exists if we're planning on swapping functionality later. Agreed on the rest.
💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949816655)
@instagibbs Note that this is about a separate addition I made, which compares the behaviour with a pure floating-point simulation. The comparison with `CFeeRate` is elsewhere.
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949816655)
@instagibbs Note that this is about a separate addition I made, which compares the behaviour with a pure floating-point simulation. The comparison with `CFeeRate` is elsewhere.
💬 instagibbs commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949817770)
oh right, I conflated the two threads here
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1949817770)
oh right, I conflated the two threads here
💬 furszy commented on issue "Prune Node Rescan Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/29183#issuecomment-2649137150)
> Since we do need to selectively re-download specific blocks, it might be prudent to have an option only re-download those specific blocks from peers only over TOR
Thats possible for sure. Could also request a few extra blocks to make the guess harder.
> Also, can the re-downloaded blocks be re-downloaded in parallel since we will know what blocks they are?
Yes. Thats how #27837 was implemented.
(https://github.com/bitcoin/bitcoin/issues/29183#issuecomment-2649137150)
> Since we do need to selectively re-download specific blocks, it might be prudent to have an option only re-download those specific blocks from peers only over TOR
Thats possible for sure. Could also request a few extra blocks to make the guess harder.
> Also, can the re-downloaded blocks be re-downloaded in parallel since we will know what blocks they are?
Yes. Thats how #27837 was implemented.
💬 brunoerg commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2649180340)
friendly ping: @murchandamus
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2649180340)
friendly ping: @murchandamus
👍 stickies-v approved a pull request: "[27.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/31422#pullrequestreview-2607160362)
ACK 25f150304d01940d7936125b7b4bf8e6c01d35aa
I've verified that all backported commits are clean, and they seem sensible. The CI failure seems unrelated, and I could not reproduce locally with `./test/functional/test_runner.py --previous-releases --filter wallet_upgradewallet`. It was reported in https://github.com/bitcoin/bitcoin/issues/31210 and fixed in https://github.com/bitcoin/bitcoin/pull/30125, but backporting the (quite extensive) fix commit probably is not necessary for this PR?
(https://github.com/bitcoin/bitcoin/pull/31422#pullrequestreview-2607160362)
ACK 25f150304d01940d7936125b7b4bf8e6c01d35aa
I've verified that all backported commits are clean, and they seem sensible. The CI failure seems unrelated, and I could not reproduce locally with `./test/functional/test_runner.py --previous-releases --filter wallet_upgradewallet`. It was reported in https://github.com/bitcoin/bitcoin/issues/31210 and fixed in https://github.com/bitcoin/bitcoin/pull/30125, but backporting the (quite extensive) fix commit probably is not necessary for this PR?
💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2649234327)
Pushed a fix for the wrong architecture detection.
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2649234327)
Pushed a fix for the wrong architecture detection.
💬 yancyribbens commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2649274408)
Change is computed as:
```
effective_values - target - change_fee
1395186823938825 - 1395186823938466 - 155
204
```
Why does the test assert this should be zero instead of 204?
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2649274408)
Change is computed as:
```
effective_values - target - change_fee
1395186823938825 - 1395186823938466 - 155
204
```
Why does the test assert this should be zero instead of 204?
🤔 instagibbs reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2606517127)
The resource bounds additions seem to make sense, still working through the workset change implications.
I've got a minimal fuzz harness checking that the "honest" peer cannot be evicted, please feel free to take it: https://github.com/instagibbs/bitcoin/tree/2025-01-orphanage-peer-dos_greg_2
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2606517127)
The resource bounds additions seem to make sense, still working through the workset change implications.
I've got a minimal fuzz harness checking that the "honest" peer cannot be evicted, please feel free to take it: https://github.com/instagibbs/bitcoin/tree/2025-01-orphanage-peer-dos_greg_2
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949514690)
s/may provide/may provide without risking eviction/?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949514690)
s/may provide/may provide without risking eviction/?
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949514831)
s/allocation/protected allocation/?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949514831)
s/allocation/protected allocation/?
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949498021)
ef2f44e653a4877d4e65fbd5a51ec83ceb96d212
`MAX_GLOBAL_ORPHAN_ANNOUNCEMENTS` doesn't occur in the PR, should be `DEFAULT_MAX_ORPHAN_ANNOUNCEMENTS`?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949498021)
ef2f44e653a4877d4e65fbd5a51ec83ceb96d212
`MAX_GLOBAL_ORPHAN_ANNOUNCEMENTS` doesn't occur in the PR, should be `DEFAULT_MAX_ORPHAN_ANNOUNCEMENTS`?
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949538646)
note: this is use to "normalize" the one DoS score metric against the other only. If this wasn't used, trimming would heavily favor announcement trimming scores first.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949538646)
note: this is use to "normalize" the one DoS score metric against the other only. If this wasn't used, trimming would heavily favor announcement trimming scores first.
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949519057)
do we think we'll end up using the derived class in a meaningful way later?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949519057)
do we think we'll end up using the derived class in a meaningful way later?
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949554535)
re:global memory limits, they won't be increased until each connected peer offers up their own orphan or announcement (which makes sense)
would be good to have test/fuzz coverage that there isn't some bug where this continuously grows, raising the global limits to unsafe levels? Arg in `SanityCheck`?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949554535)
re:global memory limits, they won't be increased until each connected peer offers up their own orphan or announcement (which makes sense)
would be good to have test/fuzz coverage that there isn't some bug where this continuously grows, raising the global limits to unsafe levels? Arg in `SanityCheck`?
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949583127)
wording confusion: same peer change?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949583127)
wording confusion: same peer change?
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949612609)
nit: would rather we check that we evicted the dos'y peer and somehow didn't *add* more resources allocated to him
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949612609)
nit: would rather we check that we evicted the dos'y peer and somehow didn't *add* more resources allocated to him
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949568939)
Maybe a bit too paranoid since it was just computed explicitly from the underlying list?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1949568939)
Maybe a bit too paranoid since it was just computed explicitly from the underlying list?
💬 yancyribbens commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2649282877)
In otherwords, I think setting the change_fee to 359 would produce zero change, although I'm not sure what you are testing.
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2649282877)
In otherwords, I think setting the change_fee to 359 would produce zero change, although I'm not sure what you are testing.