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

💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1433329273)
I pushed some changes (with @Xekyo's permission, thanks):
- Capped traversal at 500 items in `CalculateCluster()`. Number is arbitrary, open for commentary
- Fixed up a few things in the MiniMiner implementation, mostly shuffling things around and updating comments
- Dropped the chain interface changes (I think those can go in #26152)
- Expanded unit tests
- Added a fuzzer (expanded from @dergoegge's, thanks)
- Added a fuzzer to differentially test block templates built by `BlockAssembler`
...

💬 pinheadmz commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-1433331637)
Would this change need a `release-notes-27064.md` file? It is kinda changing a configuration model

📝 TheCharlatan review_request_removed a pull request: "refactor / kernel: Move non-gArgs chainparams functionality to kernel"
(https://github.com/bitcoin/bitcoin/pull/26177)
This pull request is mostly a code-move of chainparams related functionality into the kernel library. @dongcarl is the original author of this patchset, these commits were taken from https://github.com/dongcarl/bitcoin/tree/2022-03-libbitcoinkernel-chainparams-args-only.

#### Context

The bitcoin kernel library currently relies on code containing user configurations through the `ArgsManager`. This is not optimal, since as a stand-alone library it should not rely on most of the user-define
...

📝 TheCharlatan review_request_removed a pull request: "refactor / kernel: Move non-gArgs chainparams functionality to kernel"
(https://github.com/bitcoin/bitcoin/pull/26177)
This pull request is mostly a code-move of chainparams related functionality into the kernel library. @dongcarl is the original author of this patchset, these commits were taken from https://github.com/dongcarl/bitcoin/tree/2022-03-libbitcoinkernel-chainparams-args-only.

#### Context

The bitcoin kernel library currently relies on code containing user configurations through the `ArgsManager`. This is not optimal, since as a stand-alone library it should not rely on most of the user-define
...

💬 MarcoFalke commented on pull request "lint: enable E722 do not use bare except":
(https://github.com/bitcoin/bitcoin/pull/25867#issuecomment-1433342403)
> bare excepts should be replaced with specific exceptions, not except Exception.

I wonder if this should be done.

At least it is unclear to me how to review this easily. And if it is hard to review and maintain, I wonder if it is worth it.

💬 hebasto commented on pull request "src/randomenv.cpp: fix build on uclibc":
(https://github.com/bitcoin/bitcoin/pull/20358#issuecomment-1433349312)
Hmm, this patch looks [buggy](https://github.com/bitcoin/bitcoin/pull/25302#issuecomment-1433291262)...