👍 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.
💬 brunoerg commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2649284322)
This is a failing test to show BnB is producing change.
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2649284322)
This is a failing test to show BnB is producing change.
⚠️ darosior opened an issue: "`rpc_getblockstats.py` fails with `--gen-test-data`"
(https://github.com/bitcoin/bitcoin/issues/31838)
The test only pass with the currently generated data. It will fail when using the integrated feature to generate this data and running against it.
Reproduction instructions. On top of current master 1d813e4bf52a207ec526303df7b2e3d40d5eaa54 create a clean default build and run the test:
```
cmake -B repro
cmake --build repro -j20
./repro/test/functional/rpc_getblockstats.py --gen-test-data
```
Initially it will fail because it's missing the wallet arguments. Patch it with:
```diff
diff --git a/
...
(https://github.com/bitcoin/bitcoin/issues/31838)
The test only pass with the currently generated data. It will fail when using the integrated feature to generate this data and running against it.
Reproduction instructions. On top of current master 1d813e4bf52a207ec526303df7b2e3d40d5eaa54 create a clean default build and run the test:
```
cmake -B repro
cmake --build repro -j20
./repro/test/functional/rpc_getblockstats.py --gen-test-data
```
Initially it will fail because it's missing the wallet arguments. Patch it with:
```diff
diff --git a/
...
💬 yancyribbens commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2649290842)
> This is a failing test to show BnB is producing change.
With those parameters though, BnB should be producing change.
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2649290842)
> This is a failing test to show BnB is producing change.
With those parameters though, BnB should be producing change.
👍 instagibbs approved a pull request: "test: add missing sync to p2p_tx_download.py"
(https://github.com/bitcoin/bitcoin/pull/31837#pullrequestreview-2607241569)
ACK 8fe552fe6e0f1fb3600d4c5bc53b4873def6e94e
(https://github.com/bitcoin/bitcoin/pull/31837#pullrequestreview-2607241569)
ACK 8fe552fe6e0f1fb3600d4c5bc53b4873def6e94e
💬 instagibbs commented on pull request "test: add missing sync to p2p_tx_download.py":
(https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1949936893)
could also `send_and_ping`
(https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1949936893)
could also `send_and_ping`
🤔 sipa reviewed a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2607272614)
Code review ACK af76664b12d8611b606a7e755a103a20542ee539; I did not review the tests in detail.
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2607272614)
Code review ACK af76664b12d8611b606a7e755a103a20542ee539; I did not review the tests in detail.