💬 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
...
💬 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-1809666676)
Other than that a47f220933f202dd1f972d30f6cb31b09ca0a225 works. Without it, when migrating a watch-only wallet it rescans from the wallet birth height. With it, it does not rescan.
Regarding 65b2da99fe18768d1bd4cd67522f7031f16a3420 this gets a bit unwieldy because there are many places the migration can fail. Maybe just let the caller `MigrateLegacyToDescriptor` attempt a reload if migration failed.
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1809666676)
Other than that a47f220933f202dd1f972d30f6cb31b09ca0a225 works. Without it, when migrating a watch-only wallet it rescans from the wallet birth height. With it, it does not rescan.
Regarding 65b2da99fe18768d1bd4cd67522f7031f16a3420 this gets a bit unwieldy because there are many places the migration can fail. Maybe just let the caller `MigrateLegacyToDescriptor` attempt a reload if migration failed.
💬 hebasto commented on issue "macOS qt QTimer::stop crash on v26.0rc2":
(https://github.com/bitcoin/bitcoin/issues/28867#issuecomment-1809792317)
> I believe I was closing the application (according to the stacktrace)
Is it reproducible?
(https://github.com/bitcoin/bitcoin/issues/28867#issuecomment-1809792317)
> I believe I was closing the application (according to the stacktrace)
Is it reproducible?
💬 willcl-ark commented on issue "26.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/28866#issuecomment-1809819025)
# V26.0 Testing Guide notes
Hey @m3dwards thanks for taking this on, the guide looks great so far! I didn't practically run through all the tests myself yet, but left a few tiny nits from a read-through below:
## Compile Release Candidate
> it is recommended that you compile with sqlite(--with-sqlite=yes), so make sure you have installed the sqlite3 dependency.
default is `--with-sqlite=auto` which will pick up `sqlite3` if it's installed, so this isn't particularly required so long
...
(https://github.com/bitcoin/bitcoin/issues/28866#issuecomment-1809819025)
# V26.0 Testing Guide notes
Hey @m3dwards thanks for taking this on, the guide looks great so far! I didn't practically run through all the tests myself yet, but left a few tiny nits from a read-through below:
## Compile Release Candidate
> it is recommended that you compile with sqlite(--with-sqlite=yes), so make sure you have installed the sqlite3 dependency.
default is `--with-sqlite=auto` which will pick up `sqlite3` if it's installed, so this isn't particularly required so long
...
💬 Riahiamirreza commented on pull request "rpc: show P2(W)SH redeemScript in getrawtransaction #27637":
(https://github.com/bitcoin/bitcoin/pull/27638#discussion_r1392260431)
I'll add description to description fields later.
(https://github.com/bitcoin/bitcoin/pull/27638#discussion_r1392260431)
I'll add description to description fields later.
🚀 fanquake merged a pull request: "depends: latest config.guess & config.sub"
(https://github.com/bitcoin/bitcoin/pull/28781)
(https://github.com/bitcoin/bitcoin/pull/28781)
💬 Sjors commented on pull request "guix: update signapple (drop macho & altgraph)":
(https://github.com/bitcoin/bitcoin/pull/28859#issuecomment-1809855632)
ACK f718a74b124c723548f5d1961ef4e3aa15c33847
Guix hashes for 'x86_64-apple-darwin` and `arm64-apple-darwin' (generated on x86_64 linux):
```
7ea71c9d553c8afaa6c1f8ba72b643783910a972faabd6712f85020dbe8dbdae guix-build-f718a74b124c/output/arm64-apple-darwin/SHA256SUMS.part
3263327cbd4ee22c21256410da377b38399d07f163a6d1c6c266aa1f9024de64 guix-build-f718a74b124c/output/arm64-apple-darwin/bitcoin-f718a74b124c-arm64-apple-darwin-unsigned.tar.gz
dc639d8aff10827c0e1a24e4edf16425cf08d443948659
...
(https://github.com/bitcoin/bitcoin/pull/28859#issuecomment-1809855632)
ACK f718a74b124c723548f5d1961ef4e3aa15c33847
Guix hashes for 'x86_64-apple-darwin` and `arm64-apple-darwin' (generated on x86_64 linux):
```
7ea71c9d553c8afaa6c1f8ba72b643783910a972faabd6712f85020dbe8dbdae guix-build-f718a74b124c/output/arm64-apple-darwin/SHA256SUMS.part
3263327cbd4ee22c21256410da377b38399d07f163a6d1c6c266aa1f9024de64 guix-build-f718a74b124c/output/arm64-apple-darwin/bitcoin-f718a74b124c-arm64-apple-darwin-unsigned.tar.gz
dc639d8aff10827c0e1a24e4edf16425cf08d443948659
...
👍 vasild approved a pull request: "p2p: make block download logic aware of limited peers threshold"
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1729314172)
ACK a0687ffd568b0984a8b1e51fd0061f87f25e95d7
Thanks!
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1729314172)
ACK a0687ffd568b0984a8b1e51fd0061f87f25e95d7
Thanks!