💬 jamesob commented on pull request "script: add explanation for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1433262848)
Concept ACK
Probably worth just squashing this down to 1 commit.
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1433262848)
Concept ACK
Probably worth just squashing this down to 1 commit.
⚠️ instagibbs opened an issue: "decodescript miniscript functionality tops out at 520 bytes"
(https://github.com/bitcoin/bitcoin/issues/27111)
After https://github.com/bitcoin/bitcoin/pull/27037 miniscript is extracted from compatible scripts, unless the `FillableSigningProvider::AddCScript` check for `MAX_SCRIPT_ELEMENT_SIZE` fails, which then it falls back to legacy behavior.
@achow101
(https://github.com/bitcoin/bitcoin/issues/27111)
After https://github.com/bitcoin/bitcoin/pull/27037 miniscript is extracted from compatible scripts, unless the `FillableSigningProvider::AddCScript` check for `MAX_SCRIPT_ELEMENT_SIZE` fails, which then it falls back to legacy behavior.
@achow101
💬 instagibbs commented on issue "decodescript miniscript functionality tops out at 520 bytes":
(https://github.com/bitcoin/bitcoin/issues/27111#issuecomment-1433265740)
Was testing out with a 11-of-15 script from mainnet:
```
5b21020e0338c96a8870479f2396c373cc7696ba124e8635d41b0ea581112b678172612102675333a4e4b8fb51d9d4e22fa5a8eaced3fdac8a8cbf9be8c030f75712e6af992102896807d54bc55c24981f24a453c60ad3e8993d693732288068a23df3d9f50d4821029e51a5ef5db3137051de8323b001749932f2ff0d34c82e96a2c2461de96ae56c2102a4e1a9638d46923272c266631d94d36bdb03a64ee0e14c7518e49d2f29bc401021031c41fdbcebe17bec8d49816e00ca1b5ac34766b91c9f2ac37d39c63e5e008afb2103079e252e85abffd3c401a69b087
...
(https://github.com/bitcoin/bitcoin/issues/27111#issuecomment-1433265740)
Was testing out with a 11-of-15 script from mainnet:
```
5b21020e0338c96a8870479f2396c373cc7696ba124e8635d41b0ea581112b678172612102675333a4e4b8fb51d9d4e22fa5a8eaced3fdac8a8cbf9be8c030f75712e6af992102896807d54bc55c24981f24a453c60ad3e8993d693732288068a23df3d9f50d4821029e51a5ef5db3137051de8323b001749932f2ff0d34c82e96a2c2461de96ae56c2102a4e1a9638d46923272c266631d94d36bdb03a64ee0e14c7518e49d2f29bc401021031c41fdbcebe17bec8d49816e00ca1b5ac34766b91c9f2ac37d39c63e5e008afb2103079e252e85abffd3c401a69b087
...
👍 ryanofsky approved a pull request: "validation: Improve error handling when VerifyDB fails due to insufficient dbcache"
(https://github.com/bitcoin/bitcoin/pull/25574)
(https://github.com/bitcoin/bitcoin/pull/25574)
💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1107700526)
In commit "init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache" (cf307244db97e7b9e29a3d0ff948a6fa7877b548)
This commit seems to change behavior by no longer logging `No coin database inconsistencies in last %i blocks` in the case where `skipped_l3_checks` is true. I think it would be still be good to log the number of blocks checked and that no inconsistencies were found. Perhaps the easiest way to do that would be to revert the other changes to this function in
...
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1107700526)
In commit "init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache" (cf307244db97e7b9e29a3d0ff948a6fa7877b548)
This commit seems to change behavior by no longer logging `No coin database inconsistencies in last %i blocks` in the case where `skipped_l3_checks` is true. I think it would be still be good to log the number of blocks checked and that no inconsistencies were found. Perhaps the easiest way to do that would be to revert the other changes to this function in
...
💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1108588225)
42b192f0cbf9c04da111145c921344b0881b3ce3
Would suggest dropping two `ERROR_` prefixes here, because reason for inconsistent prefixes is not very clear, and the decision about whether to treat different conditions as errors should depend on the caller, not the implementation.
I think a simple `{ SUCCESS, CORRUPTED_BLOCK_DB, INSUFFICIENT_CACHE, INSUFFICIENT_BLOCK_DATA }` result would suffice.
You could also go further and rename `INSUFFICIENT_CACHE` to `SKIPPED_L3_CHECKS` and rename `INSU
...
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1108588225)
42b192f0cbf9c04da111145c921344b0881b3ce3
Would suggest dropping two `ERROR_` prefixes here, because reason for inconsistent prefixes is not very clear, and the decision about whether to treat different conditions as errors should depend on the caller, not the implementation.
I think a simple `{ SUCCESS, CORRUPTED_BLOCK_DB, INSUFFICIENT_CACHE, INSUFFICIENT_BLOCK_DATA }` result would suffice.
You could also go further and rename `INSUFFICIENT_CACHE` to `SKIPPED_L3_CHECKS` and rename `INSU
...
💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1108613248)
In commit "validation: Change return value of VerifyDB to enum type" (422127b696f390e7cd34e897e7d74f09e3dae2cb)
In the interest of preventing future bugs where shutdown could be interpreted as success, it seems like it would be less misleading (and easy) to return a `VerifyDBResult::INTERRUPTED` error code here, analogous to `ChainstateLoadStatus::INTERRUPTED`.
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1108613248)
In commit "validation: Change return value of VerifyDB to enum type" (422127b696f390e7cd34e897e7d74f09e3dae2cb)
In the interest of preventing future bugs where shutdown could be interpreted as success, it seems like it would be less misleading (and easy) to return a `VerifyDBResult::INTERRUPTED` error code here, analogous to `ChainstateLoadStatus::INTERRUPTED`.
💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1108607226)
In commit "init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache" (cf307244db97e7b9e29a3d0ff948a6fa7877b548)
This block of code seems to duplicate a block of code in `AppInitMain`, but in the `AppInitMain` block this PR is setting `fail_on_insufficient_dbcache` to `args.IsArgSet("-checkblocks") || args.IsArgSet("-checklevel")`, while here it is being set to `false`. Probably both blocks of code should be deduplicated and call a shared function later. But for now i
...
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1108607226)
In commit "init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache" (cf307244db97e7b9e29a3d0ff948a6fa7877b548)
This block of code seems to duplicate a block of code in `AppInitMain`, but in the `AppInitMain` block this PR is setting `fail_on_insufficient_dbcache` to `args.IsArgSet("-checkblocks") || args.IsArgSet("-checklevel")`, while here it is being set to `false`. Probably both blocks of code should be deduplicated and call a shared function later. But for now i
...
💬 fanquake commented on pull request "fix: contrib: allow multi-sig binary verification":
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1433269528)
Assigned this to the 25.x milestone. We either need to make these scripts useful/usable, or we should just delete them.
> I looked at removing single-sig (legacy) verification, but I'd actually like to keep it in.
Given that the legacy signing key [has been revoked](https://lists.linuxfoundation.org/pipermail/bitcoin-core-dev/2023-February/000115.html):
> Please remove it from verification pipelines.
probably makes more sense to remove.
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1433269528)
Assigned this to the 25.x milestone. We either need to make these scripts useful/usable, or we should just delete them.
> I looked at removing single-sig (legacy) verification, but I'd actually like to keep it in.
Given that the legacy signing key [has been revoked](https://lists.linuxfoundation.org/pipermail/bitcoin-core-dev/2023-February/000115.html):
> Please remove it from verification pipelines.
probably makes more sense to remove.
💬 sipa commented on pull request "script: add explanation for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108648411)
Maybe for `OP_0`, `OP_1`..`OP_16`, `OP_1NEGATE`, elaborate a bit by stating they they push an element onto the stack, and perhaps give the exact byte encoding rather than just the number.
E.g. `OP_3 = 0x53, // Push "\x03" onto the stack (which is interpreted as 3 by numerical opcodes)`
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108648411)
Maybe for `OP_0`, `OP_1`..`OP_16`, `OP_1NEGATE`, elaborate a bit by stating they they push an element onto the stack, and perhaps give the exact byte encoding rather than just the number.
E.g. `OP_3 = 0x53, // Push "\x03" onto the stack (which is interpreted as 3 by numerical opcodes)`
💬 sipa commented on pull request "script: add explanation for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108652111)
I can't really parse what you're trying to say here.
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108652111)
I can't really parse what you're trying to say here.
💬 sipa commented on pull request "script: add explanation for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108652728)
Maybe add whether or not it also applies in unexecuted branches?
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108652728)
Maybe add whether or not it also applies in unexecuted branches?
💬 sipa commented on pull request "script: add explanation for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108653671)
"second-top" sounds a bit strange to me; perhaps "second from the top"?
Alternatively: "remove the two top stack items" ?
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108653671)
"second-top" sounds a bit strange to me; perhaps "second from the top"?
Alternatively: "remove the two top stack items" ?
💬 sipa commented on pull request "script: add explanation for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108656534)
Perhaps clarify that this does not pop the top element whose size is inspected.
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108656534)
Perhaps clarify that this does not pop the top element whose size is inspected.
💬 sipa commented on pull request "script: add explanation for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108654824)
Perhaps add whether this also applies in unexecuted branches? What does the unconditionally refer to?
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108654824)
Perhaps add whether this also applies in unexecuted branches? What does the unconditionally refer to?
💬 sipa commented on pull request "script: add explanation for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108651477)
Also, perhaps use `/**< ... */` or `//!< ..` so that doxygen will pick the descriptions up.
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1108651477)
Also, perhaps use `/**< ... */` or `//!< ..` so that doxygen will pick the descriptions up.
💬 ryanofsky commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1108655998)
In commit "init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache" (cf307244db97e7b9e29a3d0ff948a6fa7877b548)
`true` might be a safer default for libbitcoinkernel. Also the name of this option `ChainstateLoadOptions::fail_on_insufficient_dbcache` seems a little misleading because it seems like it is referring to insuffcient dbcache for loading, not insufficient_dbcache for verification. Would maybe change it to `ChainstateLoadOptions::require_full_verification` wit
...
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1108655998)
In commit "init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache" (cf307244db97e7b9e29a3d0ff948a6fa7877b548)
`true` might be a safer default for libbitcoinkernel. Also the name of this option `ChainstateLoadOptions::fail_on_insufficient_dbcache` seems a little misleading because it seems like it is referring to insuffcient dbcache for loading, not insufficient_dbcache for verification. Would maybe change it to `ChainstateLoadOptions::require_full_verification` wit
...
💬 fanquake commented on pull request "[22.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/26927#issuecomment-1433287884)
Going to merge this to unbreak building with GCC 13.x. i.e on rawhide.
(https://github.com/bitcoin/bitcoin/pull/26927#issuecomment-1433287884)
Going to merge this to unbreak building with GCC 13.x. i.e on rawhide.
💬 jamesob commented on pull request "OP_VAULT draft":
(https://github.com/bitcoin/bitcoin/pull/26857#discussion_r1108665794)
Good cleanup, done - thanks.
(https://github.com/bitcoin/bitcoin/pull/26857#discussion_r1108665794)
Good cleanup, done - thanks.
💬 hebasto commented on pull request "build: Check usages of #if defined(...)":
(https://github.com/bitcoin/bitcoin/pull/25302#issuecomment-1433291262)
Another bug found, which needs to be addressed.
The following code
https://github.com/bitcoin/bitcoin/blob/75f0e0b607cd7ff7afd56853eb34a2b285b22ad2/configure.ac#L1251-L1259
always defines the `HAVE_STRONG_GETAUXVAL` macro.
So the following `#if defined` directives can't handle it properly:
https://github.com/bitcoin/bitcoin/blob/75f0e0b607cd7ff7afd56853eb34a2b285b22ad2/src/randomenv.cpp#L56-L58 and https://github.com/bitcoin/bitcoin/blob/75f0e0b607cd7ff7afd56853eb34a2b285b22ad2/src/rand
...
(https://github.com/bitcoin/bitcoin/pull/25302#issuecomment-1433291262)
Another bug found, which needs to be addressed.
The following code
https://github.com/bitcoin/bitcoin/blob/75f0e0b607cd7ff7afd56853eb34a2b285b22ad2/configure.ac#L1251-L1259
always defines the `HAVE_STRONG_GETAUXVAL` macro.
So the following `#if defined` directives can't handle it properly:
https://github.com/bitcoin/bitcoin/blob/75f0e0b607cd7ff7afd56853eb34a2b285b22ad2/src/randomenv.cpp#L56-L58 and https://github.com/bitcoin/bitcoin/blob/75f0e0b607cd7ff7afd56853eb34a2b285b22ad2/src/rand
...
🚀 fanquake merged a pull request: "[22.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/26927)
(https://github.com/bitcoin/bitcoin/pull/26927)