💬 hebasto commented on pull request "Disable and uncheck blank when private keys are disabled":
(https://github.com/bitcoin-core/gui/pull/739#issuecomment-1590830348)
cc @furszy @ryanofsky @S3RK

💬 hebasto commented on pull request "Add menu option to migrate a wallet":
(https://github.com/bitcoin-core/gui/pull/738#issuecomment-1590837892)
Usually, we submit changes to non-`qt` subdirectory, i.e., the first commit of this PR, to the main repo, no?

💬 hebasto commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-1590840776)
@hernanmarino Are you still working on this PR?

💬 MarcoFalke commented on pull request "Add menu option to migrate a wallet":
(https://github.com/bitcoin-core/gui/pull/738#issuecomment-1590849795)
> Usually, we submit changes to non-qt subdirectory, i.e., the first commit of this PR, to the main repo, no?

I don't think this rule applies to interface changes that only affect the gui. Otherwise, it just seems like extra work for reviewers and authors to artificially rip stuff apart for no good reason, with the extra risk of ending up merging just one ripped-off half, but not the other?

💬 hebasto commented on pull request "Add menu option to migrate a wallet":
(https://github.com/bitcoin-core/gui/pull/738#issuecomment-1590853447)
> > Usually, we submit changes to non-qt subdirectory, i.e., the first commit of this PR, to the main repo, no?
>
> I don't think this rule applies to interface changes that only affect the gui. Otherwise, it just seems like extra work for reviewers and authors to artificially rip stuff apart for no good reason, with the extra risk of ending up merging just one ripped-off half, but not the other?

I agree. It would be better to just document this case for further references.

💬 hebasto commented on pull request "Add menu option to migrate a wallet":
(https://github.com/bitcoin-core/gui/pull/738#issuecomment-1590853884)
Concept ACK.

💬 hebasto commented on pull request "Add menu option to migrate a wallet":
(https://github.com/bitcoin-core/gui/pull/738#issuecomment-1590855116)
cc @furszy @ryanofsky @S3RK

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1229363976)
Then let's at least document in `importdescriptors` and `importmulti` that for any multipath descriptor with 2 paths, the second will be imported as internal?

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1590914023)
ACK 15db5e0c1c03a65db4ed5b9a400817bf96237590

CI jobs that are failing seem spurious ("Failed to start an instance! Failed to pull null image! Repository does not exist or may require authentication."), or https://github.com/bitcoin/bitcoin/issues/27879.

📝 MarcoFalke opened a pull request: " test: Use TestNode datadir_path or chain_path where possible"
(https://github.com/bitcoin/bitcoin/pull/27884)
It seems inconsistent, fragile and verbose to:

* Call `get_datadir_path` to recreate the path that already exists as field in TestNode
* Call `os.path.join` with the hardcoded chain name or `self.chain` to recreate the TestNode `chain_path` property
* Sometimes even use the hardcoded node dir name (`"node0"`)

Fix all issues by using the TestNode properties.

💬 fanquake commented on pull request "lint: suppress pip spam":
(https://github.com/bitcoin/bitcoin/pull/27871#discussion_r1229385348)
Just dropped the second commit for now.

💬 MarcoFalke commented on pull request "lint: suppress pip spam":
(https://github.com/bitcoin/bitcoin/pull/27871#discussion_r1229391879)
I don't think the warning is worth it to be ignored. There are hundreds of lines printed anyway for installing packages so removing one line, which is a warning doesn't seem too useful?

Again, if you want to print less lines, I think you can just combine the pip calls into one instead of doing this change.

✅ fanquake closed a pull request: "lint: suppress pip spam"
(https://github.com/bitcoin/bitcoin/pull/27871)

💬 MarcoFalke commented on pull request "lint: suppress pip spam":
(https://github.com/bitcoin/bitcoin/pull/27871#discussion_r1229396734)
Also, if there was any value in removing lines, you can fix the typos to remove three lines:

```
src/addrdb.cpp:213: unneccessary ==> unnecessary
src/test/miniminer_tests.cpp:466: indeces ==> indices
src/test/miniminer_tests.cpp:467: indeces ==> indices

💬 MarcoFalke commented on issue "arm64 CI failure":
(https://github.com/bitcoin/bitcoin/issues/27879#issuecomment-1590963200)
My suggestion would be to wait 24 hours to see if the issue fixes itself. If not, replace the arm64 hardware with amd64, and run everything in qemu-arm again.

💬 hebasto commented on pull request "ci: Add missing set -e to 01_base_install.sh":
(https://github.com/bitcoin/bitcoin/pull/27739#issuecomment-1590967370)
> > `+ [[ true == \t\r\u\e ]]`
>
> Unrelated: Anyone know why it prints "true" as "\t\r\u\e"? Maybe we should remove bash, but I am not sure if there is an alternative.

There are two options:
- remove quoting:
```diff
--- a/ci/test/01_base_install.sh
+++ b/ci/test/01_base_install.sh
@@ -70,7 +70,7 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
ninja -C "${BASE_SCRATCH_DIR}"/msan/cxx_build/ "$MAKEJOBS"
fi

-if [[ "${RUN_TIDY}" == "true" ]]; then
+if [[ $RUN_TIDY == true
...

💬 hebasto commented on issue "arm64 CI failure":
(https://github.com/bitcoin/bitcoin/issues/27879#issuecomment-1590979307)
> There is certainly something sketchy going on with the upstream hardware...

It looks like the underlying hardware stopped to support running ARM-32bit binaries on `arm64` hardware. Actually, there was not such a promise from Cirrus Labs about that.

> My suggestion would be to wait 24 hours to see if the issue fixes itself. If not, replace the arm64 hardware with amd64, and run everything in qemu-arm again.

Another option is to build `aarch64` binaries using an `arm_container` in this
...

👋 pinheadmz's pull request is ready for review: "test: Add unit & functional test coverage for blockstore"
(https://github.com/bitcoin/bitcoin/pull/27850)

💬 MarcoFalke commented on issue "arm64 CI failure":
(https://github.com/bitcoin/bitcoin/issues/27879#issuecomment-1590995412)
Thanks for taking a look, do you mind updating the CI task config for aarch64? https://github.com/bitcoin/bitcoin/blob/427853ab49f610e971b73ea4cc1d5366747e52b1/ci/test/00_setup_env_arm.sh#L9-L14

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1229432536)
The A <- B <- C example is handled without the diamond check, yes. Accept A, reject B, reject B+C. I'm pretty sure the code on master would do this if you removed the child-with-parents requirement.

The case in the unit test isn't handled by validating subpackages, no.
I agree the "no parent pay for child" rule is not enough in some cases, and is too restrictive in others.

For example, let's say the minfeerate is 2sat/vB:

```
A1 A2 (both 1sat/vB)
\ / \

...

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1590996951)
Ready for review now, CI is passing modulo https://github.com/bitcoin/bitcoin/issues/27879