💬 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`.

💬 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
...

💬 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.

💬 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)`

💬 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.

💬 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?

💬 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" ?

💬 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.

💬 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?

💬 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.

💬 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
...

💬 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.

💬 jamesob commented on pull request "OP_VAULT draft":
(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
...

🚀 fanquake merged a pull request: "[22.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/26927)

💬 jamesob commented on pull request "fix: contrib: allow multi-sig binary verification":
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1433301398)
> We either need to make these scripts useful/usable, or we should just delete them.

Agreed - right now in master this script is useless for contemporary versions.

Unless anyone thinks otherwise, I think the best way forward here is for me to remove support for pre-multi-sig binaries and compress this change down to a single commit for easier review.

💬 MarcoFalke commented on pull request "validation: Improve error handling when VerifyDB fails due to insufficient dbcache":
(https://github.com/bitcoin/bitcoin/pull/25574#discussion_r1108687736)
Maybe just drop the hunk? Shouldn't matter either way :man_shrugging:

💬 fanquake commented on pull request "[23.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/26921#issuecomment-1433314002)
Also merging this to unbreak the GCC 13 build.

💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1433315562)
Thanks for the review. [diff](https://github.com/bitcoin/bitcoin/compare/f54210f50ec0cd44f026988308d975397a948d8c..1f8c242e07a0842a3c7cc8735a6dde3e92327bb4)

🚀 fanquake merged a pull request: "[23.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/26921)