💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2811266661)
> Can you add a test for for failing to migrate a wallet specified by relative path? It's possible that restoration won't work as we expect it to.

Added a test that the 'automatic' restoration from a backup after a failed migration works, and added a test that restoring a backup made from an external relative wallet works.

> Also IIUC, there wasn't an issue with relative paths generally like "a/b/c", only an issue with relative paths containing `..` pointing outside of the wallets director
...

💬 davidgumberg commented on pull request "bench: Fix WalletMigration benchmark":
(https://github.com/bitcoin/bitcoin/pull/32281#discussion_r2047985458)
For context on my comment in https://github.com/bitcoin/bitcoin/issues/32277#issuecomment-2807698798, `SetAddressBook()` includes a [write](https://github.com/bitcoin/bitcoin/blob/e66e30c9e5383a467789574e61114b57536193b9/src/wallet/wallet.cpp#L2496-L2500) to the database, and so does [`AddToWallet(tx)`](https://github.com/bitcoin/bitcoin/blob/e66e30c9e5383a467789574e61114b57536193b9/src/wallet/wallet.cpp#L1160)

This is the reason why some records were persisted, but none that were strictly
...

💬 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.

💬 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)
```

💬 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.

🤔 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).

💬 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
...

🤔 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".

💬 w0xlt commented on pull request "qt, wallet: Convert uint256 to Txid":
(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
...

💬 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
...

🤔 furszy reviewed a pull request: "bench: Fix WalletMigration benchmark"
(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

💬 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

💬 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
...

🤔 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..!

💬 shahsb commented on pull request "BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only)":
(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.

💬 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?

💬 vasild commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#discussion_r2048389017)
Done.