💬 w0xlt commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2047995904)
nit: Since `coinselection_tests.cpp` is introduced in the first commit, this change may be there as well.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2047995904)
nit: Since `coinselection_tests.cpp` is introduced in the first commit, this change may be there as well.
💬 w0xlt commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2048001478)
nit: Just for clarity
```suggestion
static std::string InputAmountsToString(const SelectionResult& selection)
```
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2048001478)
nit: Just for clarity
```suggestion
static std::string InputAmountsToString(const SelectionResult& selection)
```
💬 w0xlt commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2048003665)
nit: This function is added in the commit https://github.com/bitcoin/bitcoin/pull/29532/commits/66200b3ffa21605fc3234ccbda7b424381f3319a and then modified in the commit https://github.com/bitcoin/bitcoin/pull/29532/commits/afd4b807ff1300e4f74ceab6a683f3ff1376369d.
It can be added directly as it is in the latest commit.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2048003665)
nit: This function is added in the commit https://github.com/bitcoin/bitcoin/pull/29532/commits/66200b3ffa21605fc3234ccbda7b424381f3319a and then modified in the commit https://github.com/bitcoin/bitcoin/pull/29532/commits/afd4b807ff1300e4f74ceab6a683f3ff1376369d.
It can be added directly as it is in the latest commit.
🤔 w0xlt reviewed a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2774237655)
nit: Perhaps the commit descriptions ("BnB rate sensitivity tests", "simple BnB failure tests", and "BnB iteration exhaustion test", for example) could become functions for clarity (not necessarily new test cases).
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2774237655)
nit: Perhaps the commit descriptions ("BnB rate sensitivity tests", "simple BnB failure tests", and "BnB iteration exhaustion test", for example) could become functions for clarity (not necessarily new test cases).
💬 davidgumberg commented on pull request "bench: Fix WalletMigration benchmark":
(https://github.com/bitcoin/bitcoin/pull/32281#issuecomment-2811361979)
Tested ACK 7912cd41258d59e4
Either of the two DB writes added here seems to fix the issue in my testing, but it's better to be consistent and write both.
---
I am no longer confident the [change to `success`](https://github.com/bitcoin/bitcoin/pull/32149/commits/0f602c5693ef5d0c63b1e5b7dc0990dced3655d6#diff-1f2db0e4d5c12d109c7f0962333c245b49b696cb39ff432da048e9d6c08944d8L4537-R4543) introduced in #32149 makes sense:
https://github.com/bitcoin/bitcoin/blob/e66e30c9e5383a467789574e61
...
(https://github.com/bitcoin/bitcoin/pull/32281#issuecomment-2811361979)
Tested ACK 7912cd41258d59e4
Either of the two DB writes added here seems to fix the issue in my testing, but it's better to be consistent and write both.
---
I am no longer confident the [change to `success`](https://github.com/bitcoin/bitcoin/pull/32149/commits/0f602c5693ef5d0c63b1e5b7dc0990dced3655d6#diff-1f2db0e4d5c12d109c7f0962333c245b49b696cb39ff432da048e9d6c08944d8L4537-R4543) introduced in #32149 makes sense:
https://github.com/bitcoin/bitcoin/blob/e66e30c9e5383a467789574e61
...
🤔 w0xlt reviewed a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2774238544)
nit: Some commits can be squashed to avoid warnings like "unused function".
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2774238544)
nit: Some commits can be squashed to avoid warnings like "unused function".
💬 w0xlt commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2811380510)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2811380510)
Approach ACK
💬 davidgumberg commented on pull request "contrib: refactor: dedup deserialization routines in utxo-to-sqlite script":
(https://github.com/bitcoin/bitcoin/pull/32116#issuecomment-2811396408)
Another interesting user of the functional test framework is the warnet tool: https://github.com/bitcoin-dev-project/warnet.
I suspect one challenge/tradeoff with an additional common framework/interface is that this would increase maintenance burden today with only a hypothetical future benefit, the utility framework might languish featureless and unmaintained and/or duplicating things more first-class external interfaces like the kernel do, and doing things the other way around trades a fut
...
(https://github.com/bitcoin/bitcoin/pull/32116#issuecomment-2811396408)
Another interesting user of the functional test framework is the warnet tool: https://github.com/bitcoin-dev-project/warnet.
I suspect one challenge/tradeoff with an additional common framework/interface is that this would increase maintenance burden today with only a hypothetical future benefit, the utility framework might languish featureless and unmaintained and/or duplicating things more first-class external interfaces like the kernel do, and doing things the other way around trades a fut
...
💬 pseudoramdom commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2811571133)
Tested ACK https://github.com/bitcoin/bitcoin/pull/32273/commits/db4ea2632f0c1a5a5d6885526545cbddae7305d7
The patch works for both paths containing `"../"` and `"a/b/c/d"`
> I thought so too, but checked after reading your comment and this is also broken on master:
+1. `master` does fail `migrate` for `"a/b/c"` style.
<details>
<summary>master</summary>
Migrating with `..` style
```
2025-04-17T01:49:47Z copied /Library/Application Support/Bitcoin/regtest/wallets/../../../myrelat
...
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2811571133)
Tested ACK https://github.com/bitcoin/bitcoin/pull/32273/commits/db4ea2632f0c1a5a5d6885526545cbddae7305d7
The patch works for both paths containing `"../"` and `"a/b/c/d"`
> I thought so too, but checked after reading your comment and this is also broken on master:
+1. `master` does fail `migrate` for `"a/b/c"` style.
<details>
<summary>master</summary>
Migrating with `..` style
```
2025-04-17T01:49:47Z copied /Library/Application Support/Bitcoin/regtest/wallets/../../../myrelat
...
🤔 furszy reviewed a pull request: "bench: Fix WalletMigration benchmark"
(https://github.com/bitcoin/bitcoin/pull/32281#pullrequestreview-2774369418)
utACK 7912cd41258d5
(https://github.com/bitcoin/bitcoin/pull/32281#pullrequestreview-2774369418)
utACK 7912cd41258d5
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2811720979)
`8939947449...aa88c6dfb6`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2811720979)
`8939947449...aa88c6dfb6`: rebase due to conflicts
💬 achow101 commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2811880334)
A user has reported their router is (probably) incompatible: https://bitcointalk.org/index.php?topic=5538256.msg65285896#msg65285896
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2811880334)
A user has reported their router is (probably) incompatible: https://bitcointalk.org/index.php?topic=5538256.msg65285896#msg65285896
💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2048325592)
Writing as a review comment since I didn't think a code comment was warranted here, the reason for putting this on a separate line even though it is only used once is to avoid the wrath of the slightly [imprecise](https://github.com/bitcoin/bitcoin/blob/e66e30c9e5383a467789574e61114b57536193b9/test/lint/lint-python-utf8-encoding.py#L31) python-utf8-encoding [lint check](https://github.com/bitcoin/bitcoin/blob/e66e30c9e5383a467789574e61114b57536193b9/test/lint/lint-python-utf8-encoding.py) which
...
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2048325592)
Writing as a review comment since I didn't think a code comment was warranted here, the reason for putting this on a separate line even though it is only used once is to avoid the wrath of the slightly [imprecise](https://github.com/bitcoin/bitcoin/blob/e66e30c9e5383a467789574e61114b57536193b9/test/lint/lint-python-utf8-encoding.py#L31) python-utf8-encoding [lint check](https://github.com/bitcoin/bitcoin/blob/e66e30c9e5383a467789574e61114b57536193b9/test/lint/lint-python-utf8-encoding.py) which
...
🤔 shahsb reviewed a pull request: "BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only)"
(https://github.com/bitcoin/bitcoin/pull/32247#pullrequestreview-2774788946)
I agree with @JeremyRubin comments..!
(https://github.com/bitcoin/bitcoin/pull/32247#pullrequestreview-2774788946)
I agree with @JeremyRubin comments..!
💬 shahsb commented on pull request "BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only)":
(https://github.com/bitcoin/bitcoin/pull/32247#issuecomment-2811995385)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32247#issuecomment-2811995385)
Concept ACK
💬 maflcko commented on issue "ci: fuzz_with_valgrind job broken":
(https://github.com/bitcoin/bitcoin/issues/32276#issuecomment-2812024548)
I tried `creduce`, but at some point it seems to have transformed the false positive warning into a true positive warning.
(https://github.com/bitcoin/bitcoin/issues/32276#issuecomment-2812024548)
I tried `creduce`, but at some point it seems to have transformed the false positive warning into a true positive warning.
💬 vasild commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2812025976)
`9cc8ca3b1f...bb822a5ee1`: address suggestions
I think the `bitcoin-cli` extension in https://github.com/jonatack/bitcoin/commit/708a9502f8eca7aaa84236ea038a574f4350f298#r155516952 might be included in this PR. @i-am-yuvi, since you ACKed this PR without that extra commit, what do you think? Should it be included?
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2812025976)
`9cc8ca3b1f...bb822a5ee1`: address suggestions
I think the `bitcoin-cli` extension in https://github.com/jonatack/bitcoin/commit/708a9502f8eca7aaa84236ea038a574f4350f298#r155516952 might be included in this PR. @i-am-yuvi, since you ACKed this PR without that extra commit, what do you think? Should it be included?
💬 vasild commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048389017)
Done.
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048389017)
Done.
💬 vasild commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048389405)
Done.
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048389405)
Done.
💬 vasild commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048392905)
Shortened a bit but not that much since this is the only help text (I assume release notes are not so useful for somebody looking into this for the first time in e.g. version 35.0, they will not go to search for past release notes).
Other help texts include `\n`, maybe do that here as well?
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048392905)
Shortened a bit but not that much since this is the only help text (I assume release notes are not so useful for somebody looking into this for the first time in e.g. version 35.0, they will not go to search for past release notes).
Other help texts include `\n`, maybe do that here as well?