💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833534)
Done as suggested
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833534)
Done as suggested
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833557)
Renamed as suggested
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833557)
Renamed as suggested
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833583)
Dropped
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833583)
Dropped
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833609)
Done
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833609)
Done
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833667)
Done
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833667)
Done
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833705)
Good point, changed to two functions `GetRootPubKey` and `GetRootExtPubKey`, each returning an optional `CPubKey` or `CExtPubKey`, respectively.
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833705)
Good point, changed to two functions `GetRootPubKey` and `GetRootExtPubKey`, each returning an optional `CPubKey` or `CExtPubKey`, respectively.
💬 achow101 commented on pull request "test: Generate coverage report without running tests":
(https://github.com/bitcoin/bitcoin/pull/28772#issuecomment-1809341585)
> Is it still accurate to call the coverage report `test_bitcoin` when it may cover something else?
Any suggestions?
> Do the docs need an update?
I don't think so? (besides renaming, if we do that). The way I read the docs was that `make check` had to be run first, and separately. I hadn't expected `make cov` to run them for me.
(https://github.com/bitcoin/bitcoin/pull/28772#issuecomment-1809341585)
> Is it still accurate to call the coverage report `test_bitcoin` when it may cover something else?
Any suggestions?
> Do the docs need an update?
I don't think so? (besides renaming, if we do that). The way I read the docs was that `make check` had to be run first, and separately. I hadn't expected `make cov` to run them for me.
💬 achow101 commented on pull request "tests: Fix LCOV_OPTS to be in the correct position":
(https://github.com/bitcoin/bitcoin/pull/28771#issuecomment-1809346844)
> Can you clarify whether this has any visible effect on the generated html? I presume only when branch coverage is enabled?
It shouldn't, other than actually working. The man page for `lcov` says:
-a tracefile_pattern
--add-tracefile tracefile_pattern
Add contents of all files matching glob pattern trcefile_pattern.
Specify several tracefiles using the -a switch to combine the coverage data contained in
these files by adding up
...
(https://github.com/bitcoin/bitcoin/pull/28771#issuecomment-1809346844)
> Can you clarify whether this has any visible effect on the generated html? I presume only when branch coverage is enabled?
It shouldn't, other than actually working. The man page for `lcov` says:
-a tracefile_pattern
--add-tracefile tracefile_pattern
Add contents of all files matching glob pattern trcefile_pattern.
Specify several tracefiles using the -a switch to combine the coverage data contained in
these files by adding up
...
💬 achow101 commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1809348998)
It seems like there's a random failure happening where something in BDB is `free()`ing an invalid pointer which causes bitcoind to abort. This appears to only be happening with the change the reloads the wallet for the early exits. I don't really understand why that is even happening, and it doesn't happen consistently enough to reliably debug.
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1809348998)
It seems like there's a random failure happening where something in BDB is `free()`ing an invalid pointer which causes bitcoind to abort. This appears to only be happening with the change the reloads the wallet for the early exits. I don't really understand why that is even happening, and it doesn't happen consistently enough to reliably debug.
💬 ajtowns commented on issue "Make -stopatheight work with background sync":
(https://github.com/bitcoin/bitcoin/issues/28809#issuecomment-1809360370)
Wouldn't this be better as a separate option, `-stopbackgroundatheight` or something ?
(https://github.com/bitcoin/bitcoin/issues/28809#issuecomment-1809360370)
Wouldn't this be better as a separate option, `-stopbackgroundatheight` or something ?
💬 petertodd commented on pull request "[WIP] BIP300 (Drivechains) consensus-level logic":
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1809545337)
> It isn't an issue though. h* can and in our examples does include information about which chain it belongs to.
As BIP-301 states, h* is a 32 byte hash digest. The problem is you can have two valid h*'s in the same coinbase, for the same chain, and without access to the data behind h*, you have no way of detecting that.
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1809545337)
> It isn't an issue though. h* can and in our examples does include information about which chain it belongs to.
As BIP-301 states, h* is a 32 byte hash digest. The problem is you can have two valid h*'s in the same coinbase, for the same chain, and without access to the data behind h*, you have no way of detecting that.
💬 CryptAxe commented on pull request "[WIP] BIP300 (Drivechains) consensus-level logic":
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1809556633)
The sidechains have access to the data behind h*. The mainchain doesn't care but it can also tell: https://github.com/LayerTwo-Labs/mainchain/blob/3f4d632a90ebd5eb5f94dba24384235043c34a69/src/validation.cpp#L3952-L3990 This is an example of one part of the BMM h* validation from the old version of our code but you get the idea.
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1809556633)
The sidechains have access to the data behind h*. The mainchain doesn't care but it can also tell: https://github.com/LayerTwo-Labs/mainchain/blob/3f4d632a90ebd5eb5f94dba24384235043c34a69/src/validation.cpp#L3952-L3990 This is an example of one part of the BMM h* validation from the old version of our code but you get the idea.
💬 Sjors commented on issue "Make -stopatheight work with background sync":
(https://github.com/bitcoin/bitcoin/issues/28809#issuecomment-1809558574)
Perhaps. But each height is only reached once, so I'm not sure if it's necessary.
(https://github.com/bitcoin/bitcoin/issues/28809#issuecomment-1809558574)
Perhaps. But each height is only reached once, so I'm not sure if it's necessary.
💬 Sjors commented on pull request "script, assumeutxo: Enhance validations in utxo_snapshot.sh":
(https://github.com/bitcoin/bitcoin/pull/28852#issuecomment-1809561795)
tACK 11b7269d83a56f919f9dddb7f7c72a96d428627f
(https://github.com/bitcoin/bitcoin/pull/28852#issuecomment-1809561795)
tACK 11b7269d83a56f919f9dddb7f7c72a96d428627f
💬 petertodd commented on pull request "[WIP] BIP300 (Drivechains) consensus-level logic":
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1809586628)
> The sidechains have access to the data behind h*. The mainchain doesn't care but it can also tell: https://github.com/LayerTwo-Labs/mainchain/blob/3f4d632a90ebd5eb5f94dba24384235043c34a69/src/validation.cpp#L3952-L3990 This is an example of one part of the BMM h* validation from the old version of our code but you get the idea.
That code allows for additional OP_Returns with the special BMM marker to be put in the set of coinbase outputs, beyond the set of BMMs that should be present due to
...
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1809586628)
> The sidechains have access to the data behind h*. The mainchain doesn't care but it can also tell: https://github.com/LayerTwo-Labs/mainchain/blob/3f4d632a90ebd5eb5f94dba24384235043c34a69/src/validation.cpp#L3952-L3990 This is an example of one part of the BMM h* validation from the old version of our code but you get the idea.
That code allows for additional OP_Returns with the special BMM marker to be put in the set of coinbase outputs, beyond the set of BMMs that should be present due to
...
💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1809604000)
re-utACK e3c292b298e5e6f2ec319fb93ba67fd64a0f4ac1
Also rebased #22341 after the name change.
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1809604000)
re-utACK e3c292b298e5e6f2ec319fb93ba67fd64a0f4ac1
Also rebased #22341 after the name change.
🤔 Sjors reviewed a pull request: "wallet: propagete `checkChainLimits` error message to wallet"
(https://github.com/bitcoin/bitcoin/pull/28863#pullrequestreview-1728989297)
Concept ACK.
It would be nice if the test can reach all four possible limit error messages, but if that's difficult to achieve it could wait for a followup.
(https://github.com/bitcoin/bitcoin/pull/28863#pullrequestreview-1728989297)
Concept ACK.
It would be nice if the test can reach all four possible limit error messages, but if that's difficult to achieve it could wait for a followup.
💬 Sjors commented on pull request "wallet: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1392038595)
nit: I don't think adding this variable improves readability
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1392038595)
nit: I don't think adding this variable improves readability
💬 Sjors commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1809618420)
I also got a not reproducible failure at 65b2da99fe18768d1bd4cd67522f7031f16a3420:
```
228/283 - wallet_migration.py failed, Duration: 2 s
stdout:
2023-11-14T06:24:21.154000Z TestFramework (INFO): PRNG seed is: 2952483070848634871
...
2023-11-14T06:24:22.297000Z TestFramework (INFO): Test migration of an encrypted wallet
2023-11-14T06:24:22.892000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
...
File "/home/sjors/dev/bitcoin/test/functional/wallet_m
...
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1809618420)
I also got a not reproducible failure at 65b2da99fe18768d1bd4cd67522f7031f16a3420:
```
228/283 - wallet_migration.py failed, Duration: 2 s
stdout:
2023-11-14T06:24:21.154000Z TestFramework (INFO): PRNG seed is: 2952483070848634871
...
2023-11-14T06:24:22.297000Z TestFramework (INFO): Test migration of an encrypted wallet
2023-11-14T06:24:22.892000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
...
File "/home/sjors/dev/bitcoin/test/functional/wallet_m
...
💬 Sjors commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1809641110)
While trying to test the first commit, I ran into something odd. It seems that an _empty_ watch-only wallet legacy is treated as a descriptor wallet by `migratewallet`.
1. Checkout master at 5800c558eb5efb4839ed00d6967e43306d68e1c3, compile with BDB
2. `src/bitcoind -deprecatedrpc=create_bdb`
3. `src/bitcoin-cli createwallet TestMigrate true true "" false false` (this warns that legacy wallets are deprecated)
4. Restart the node (not sure if necessary) and load the wallet again, double che
...
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1809641110)
While trying to test the first commit, I ran into something odd. It seems that an _empty_ watch-only wallet legacy is treated as a descriptor wallet by `migratewallet`.
1. Checkout master at 5800c558eb5efb4839ed00d6967e43306d68e1c3, compile with BDB
2. `src/bitcoind -deprecatedrpc=create_bdb`
3. `src/bitcoin-cli createwallet TestMigrate true true "" false false` (this warns that legacy wallets are deprecated)
4. Restart the node (not sure if necessary) and load the wallet again, double che
...