💬 achow101 commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450792644)
> Sounds like the node that made the backup holds on to it after writing? Maybe restart the node? Or copy the backup file?
We don't remove the backup. The failure is in cleaning up the restored wallet after it fails to restore. The backup file is copied to a new path, and that is what `remove_all` is trying to remove.
I think it's more likely that the database isn't fully closed yet even though the `CWallet` should be destructed.
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450792644)
> Sounds like the node that made the backup holds on to it after writing? Maybe restart the node? Or copy the backup file?
We don't remove the backup. The failure is in cleaning up the restored wallet after it fails to restore. The backup file is copied to a new path, and that is what `remove_all` is trying to remove.
I think it's more likely that the database isn't fully closed yet even though the `CWallet` should be destructed.
💬 achow101 commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#discussion_r1450795240)
I think it would be better if we used `skip_if_no_module` so that the test is reported as skipped instead of silently "passing" on windows. Could also use `skip_if_platform_not_posix` for that.
(https://github.com/bitcoin/bitcoin/pull/29238#discussion_r1450795240)
I think it would be better if we used `skip_if_no_module` so that the test is reported as skipped instead of silently "passing" on windows. Could also use `skip_if_platform_not_posix` for that.
💬 Sjors commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450796060)
Ok, so it has nothing to do with what n0 did. I'm surprised none of the other wallet backup & restore tests have this issue. But I guess it's a different failure condition.
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450796060)
Ok, so it has nothing to do with what n0 did. I'm surprised none of the other wallet backup & restore tests have this issue. But I guess it's a different failure condition.
💬 jamesob commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#discussion_r1450796816)
Oh, good call. Will do.
(https://github.com/bitcoin/bitcoin/pull/29238#discussion_r1450796816)
Oh, good call. Will do.
💬 achow101 commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450797858)
It looks like we don't have any tests that have a failure in `CWallet::Create`, which I don't think is that surprising since generally the wallet needs to be corrupted for that to happen.
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450797858)
It looks like we don't have any tests that have a failure in `CWallet::Create`, which I don't think is that surprising since generally the wallet needs to be corrupted for that to happen.
💬 m3dwards commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889773750)
I agree with @achow101 that there looks like there is a problem with the cleanup when the wallet restore fails.
As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`. It's intended to fail and cleanup anyway and so shouldn't affect the rest of the test.
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889773750)
I agree with @achow101 that there looks like there is a problem with the cleanup when the wallet restore fails.
As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`. It's intended to fail and cleanup anyway and so shouldn't affect the rest of the test.
💬 achow101 commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889776317)
ACK f6140e47736a1b105196ba91080c7c1d203f2a77
> As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`. It's intended to fail and cleanup anyway and so shouldn't affect the rest of the test.
That failure is an important part of the test since we need to be testing that wallets don't load while background ibd is ongoing.
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889776317)
ACK f6140e47736a1b105196ba91080c7c1d203f2a77
> As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`. It's intended to fail and cleanup anyway and so shouldn't affect the rest of the test.
That failure is an important part of the test since we need to be testing that wallets don't load while background ibd is ongoing.
💬 Sjors commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889777696)
> As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`.
That would make the test worse on all platforms. The point of that `assert_raises_rpc_error` is to prevent a regression where we allow the user to restore a wallet too early in the IBD process. That's probably not a rare occurrence: get new computer, start Bitcoin Core, load snapshot, load wallet backup -> oops.
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889777696)
> As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`.
That would make the test worse on all platforms. The point of that `assert_raises_rpc_error` is to prevent a regression where we allow the user to restore a wallet too early in the IBD process. That's probably not a rare occurrence: get new computer, start Bitcoin Core, load snapshot, load wallet backup -> oops.
💬 m3dwards commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889779254)
> That failure is an important part of the test since we need to be testing that wallets don't load while background ibd is ongoing.
Understood, was thinking just as a temporary measure until the remove_all call was fixed on windows. Perhaps just disable that line on windows then.
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889779254)
> That failure is an important part of the test since we need to be testing that wallets don't load while background ibd is ongoing.
Understood, was thinking just as a temporary measure until the remove_all call was fixed on windows. Perhaps just disable that line on windows then.
💬 achow101 commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450807950)
> I think it's more likely that the database isn't fully closed yet even though the `CWallet` should be destructed.
Maybe it's because of `-unsafesqlitesync`, going to try turning that off for the test.
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450807950)
> I think it's more likely that the database isn't fully closed yet even though the `CWallet` should be destructed.
Maybe it's because of `-unsafesqlitesync`, going to try turning that off for the test.
💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1889791034)
Thank you @michaelfolkson and @Eunovo for the reviews! I agree with both of your feedback re improving `descriptors.md` docs to accompany this test. I will have a commit addressing your feedback in the coming days.
> I think it is probably overkill to create a new functional test file for this. I'm not sure why it can't go in wallet_miniscript.py
My intention with this test is to provide a "reference example" with the same motivations and flow/architecture as #22067 (and the original issue
...
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1889791034)
Thank you @michaelfolkson and @Eunovo for the reviews! I agree with both of your feedback re improving `descriptors.md` docs to accompany this test. I will have a commit addressing your feedback in the coming days.
> I think it is probably overkill to create a new functional test file for this. I'm not sure why it can't go in wallet_miniscript.py
My intention with this test is to provide a "reference example" with the same motivations and flow/architecture as #22067 (and the original issue
...
💬 Sjors commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450812828)
I wonder why it's called unsafe :-)
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450812828)
I wonder why it's called unsafe :-)
💬 alfonsoromanz commented on pull request "Fix typos":
(https://github.com/bitcoin-core/gui/pull/787#issuecomment-1889805504)
Concept ACK. Maybe it's better to squash both commits into a single one? Not sure what others think
(https://github.com/bitcoin-core/gui/pull/787#issuecomment-1889805504)
Concept ACK. Maybe it's better to squash both commits into a single one? Not sure what others think
🤔 furszy reviewed a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1818866732)
Code review ACK df30247705, nits can be disregarded.
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1818866732)
Code review ACK df30247705, nits can be disregarded.
💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450823884)
nit:
leftover; should be "import descriptor on another wallet"
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450823884)
nit:
leftover; should be "import descriptor on another wallet"
💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450823229)
nit:
leftover
```suggestion
self.log.info("Create a child tx and wait until all wallets are notified")
```
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450823229)
nit:
leftover
```suggestion
self.log.info("Create a child tx and wait until all wallets are notified")
```
💬 Sjors commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889817551)
> I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
Here's a good place to start: https://datatracker.ietf.org/doc/html/rfc7282
(Non draft) pull requests to Bitcoin Core are not a good way to find consensus. Opening before such consensus most likely decreases of anyone being motivated to review it.
> CTV had (a year of?) workshops and review with Jeremy Rubin abo
...
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889817551)
> I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
Here's a good place to start: https://datatracker.ietf.org/doc/html/rfc7282
(Non draft) pull requests to Bitcoin Core are not a good way to find consensus. Opening before such consensus most likely decreases of anyone being motivated to review it.
> CTV had (a year of?) workshops and review with Jeremy Rubin abo
...
💬 theuni commented on pull request "build: depends move macOS C(XX) FLAGS out of C & CXX":
(https://github.com/bitcoin/bitcoin/pull/29233#issuecomment-1889823070)
Hmm. I was curious to see if the compiler forwards `-mmacosx-version-min` to the linker, and it seems it does:
```bash
$ clang -mmacosx-version-min=11.2 test.c -c -o test.o
$ clang -mmacosx-version-min=11.2 test.o -o testing -v
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: arm64-apple-darwin22.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
"/Applications/Xcode.app/Contents/Developer/Toolchain
...
(https://github.com/bitcoin/bitcoin/pull/29233#issuecomment-1889823070)
Hmm. I was curious to see if the compiler forwards `-mmacosx-version-min` to the linker, and it seems it does:
```bash
$ clang -mmacosx-version-min=11.2 test.c -c -o test.o
$ clang -mmacosx-version-min=11.2 test.o -o testing -v
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: arm64-apple-darwin22.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
"/Applications/Xcode.app/Contents/Developer/Toolchain
...
🤔 mzumsande reviewed a pull request: "log mempool loading progress"
(https://github.com/bitcoin/bitcoin/pull/29227#pullrequestreview-1818890399)
tested ACK eb78ea4eebfe150bc1746282bfdad6eb0f764e3c
(https://github.com/bitcoin/bitcoin/pull/29227#pullrequestreview-1818890399)
tested ACK eb78ea4eebfe150bc1746282bfdad6eb0f764e3c
💬 reardencode commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889850492)
> > I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
>
> Here's a good place to start: https://datatracker.ietf.org/doc/html/rfc7282
>
> (Non draft) pull requests to Bitcoin Core are not a good way to find consensus. Opening before such consensus most likely decreases of anyone being motivated to review it.
As I mentioned on my [post on delving](https://delvingbit
...
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889850492)
> > I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
>
> Here's a good place to start: https://datatracker.ietf.org/doc/html/rfc7282
>
> (Non draft) pull requests to Bitcoin Core are not a good way to find consensus. Opening before such consensus most likely decreases of anyone being motivated to review it.
As I mentioned on my [post on delving](https://delvingbit
...