💬 Sjors commented on pull request " refactor: Make node_id a const& in RemoveBlockRequest ":
(https://github.com/bitcoin/bitcoin/pull/31282#discussion_r1839882117)
fa91206b3c570dc17ee18565ade04067a88e4ef8: this seems fine to me. The code is nicer and happens to fix a false positive.

💬 fanquake commented on pull request "guix: scope pkg-config to Linux only":
(https://github.com/bitcoin/bitcoin/pull/31276#issuecomment-2473092574)
Guix build (aarch64):
```bash
02cdb4ec345e0e4b2f3876dded48a468c864fe58515ae7e4d7f9a8c1c4911337 guix-build-7edc54b086ac/output/aarch64-linux-gnu/SHA256SUMS.part
83d376375b1d5309108acc9e02711138c826d38fca0a73e7cc6048b21f3c4ab1 guix-build-7edc54b086ac/output/aarch64-linux-gnu/bitcoin-7edc54b086ac-aarch64-linux-gnu-debug.tar.gz
25677323242badfd9be8c5fdbc71ecf9b663239842deb244bbca4bc90a311d4d guix-build-7edc54b086ac/output/aarch64-linux-gnu/bitcoin-7edc54b086ac-aarch64-linux-gnu.tar.gz
de94c7
...

💬 fanquake commented on pull request "guix: scope pkg-config to Linux only":
(https://github.com/bitcoin/bitcoin/pull/31276#issuecomment-2473152977)
Dropped another patch and pruned a Qt libtool archive.

💬 fjahr commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1840013926)
reverted it

💬 fjahr commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1840014484)
done

💬 fjahr commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1840014778)
done

💬 fjahr commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#issuecomment-2473228711)
Addressed latest feedback from @maflcko

👍 vasild approved a pull request: "ci: skip Github CI on branch pushes for forks"
(https://github.com/bitcoin/bitcoin/pull/30487#pullrequestreview-2432802018)
ACK 8610bcef9d030013f9e36cffe0c58dd2cfe85d66

👍 vasild approved a pull request: "addrman: cap the `max_pct` to not exceed the maximum number of addresses"
(https://github.com/bitcoin/bitcoin/pull/31235#pullrequestreview-2432819520)
ACK 9c5775c331e02dab06c78ecbb58488542d16dda7

💬 polespinasa commented on pull request "rpc: print P2WSH witScript in getrawtransaction":
(https://github.com/bitcoin/bitcoin/pull/31252#discussion_r1840096537)
I agree, it needs to be changed in one more place apart from this line but yes I will apply it.
Thanks!

💬 naumenkogs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1840103944)
Expanding on this thought, I sketched a two-step improvement in handling single-package tx wrt code readability. Wanna [take a look](https://github.com/naumenkogs/bitcoin/commits/2024-10-submitpackage-singleton-v1/)?

I certainly might be missing something.

👍 rkrux approved a pull request: "test: Rework wallet_migration.py to use previous releases"
(https://github.com/bitcoin/bitcoin/pull/31248#pullrequestreview-2432399968)
tACK a76ad56a80d9c9a60352bb98b363131e359a383b

Certainly useful because it reflects how the end users would do wallet migration after the upcoming release.

Make is successful but around 15 functional tests are failing in my system that seem unrelated to this change because they are failing in master as well. All of the failures are due to timeouts, probably an issue with my setup.

Asked few questions and left suggestions. Found few really good tests in this file in the end!

💬 rkrux commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1839858333)
Should this not be called `create_legacy_wallet_and_get_rpc` now highlighting that it returns the wallet RPC from the old node?

💬 rkrux commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1839822834)
Not in the diff but still

```diff
- # Now migrate and test that we still see have the same balance/transactions
+ # Now migrate and test that we still have the same balance/transactions
```

💬 rkrux commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1839812117)
Nit
```
# restart master node and verify that everything is still there
```

💬 rkrux commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1839933824)
This `notloaded` wallet migration testing is removed because it is effectively tested in `test_unloaded_by_path` below?

💬 rkrux commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1839954334)
+1 for adding this. Is the reason for the absence of this _unsetting_ here not causing any issue earlier when `wallet.setmocktime(curr_time)` was added that no subsequent test in this file below relies on mock time?

💬 rkrux commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840121466)
I have noticed this pattern in most of these tests that I found a little difficult to read. Slightly more verbose naming would make it easier to parse like below. I am not suggesting this change to be done in this PR because this pattern existed earlier as well but can be updated in the future if others find this helpful as well.

```diff
- def_wallet.sendtoaddress(wallet.getnewaddress(), 10)
+ def_descriptor_wallet.sendtoaddress(legacy_wallet.getnewaddress(), 10)
```

💬 rkrux commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840131032)
Would prefer to have the commit message mention this instead of `use previous releases`, which gives an impression multiple previous releases are being used. Couple suggestions:

```diff
- test: Rework migratewallet to use previous releases
+ test: Rework migratewallet to use previous release
+ test: Rework migratewallet to use v28 release
```

💬 rkrux commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1840003618)
TIL!