💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1108608610)
I see, I'll use `dep`, because that's what is used elsewhere in the code.
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1108608610)
I see, I'll use `dep`, because that's what is used elsewhere in the code.
💬 Willtech commented on issue "Setting `onlynet=onion` still makes some IPv4 connections.":
(https://github.com/bitcoin/bitcoin/issues/12344#issuecomment-1433251695)
I only sometimes look just for interest and occasionally find this issue unless network has been updated. It is not rare it just happens when it does.
(https://github.com/bitcoin/bitcoin/issues/12344#issuecomment-1433251695)
I only sometimes look just for interest and occasionally find this issue unless network has been updated. It is not rare it just happens when it does.
💬 fanquake commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1433257883)
Moved to draft given it's based on multiple other PRs.
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1433257883)
Moved to draft given it's based on multiple other PRs.
📝 fanquake converted_to_draft a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748)
This PR introduces support for v2 P2P encryption(BIP 324) in the existing functional test framework and adds functional tests for the same.
It's built on top of:
* #24545
* #24005
The first 2 commits help the CI pass since the secp256k1 subtree directory is touched by the parent PR 24545.
The next 2 commits bring in the mentioned parent PRs. They'll all be removed when the parent PRs get merged.
### commits overview
1. Introducing cryptographic constructs(HKDF, ECDH, ChaCha20, Poly
...
(https://github.com/bitcoin/bitcoin/pull/24748)
This PR introduces support for v2 P2P encryption(BIP 324) in the existing functional test framework and adds functional tests for the same.
It's built on top of:
* #24545
* #24005
The first 2 commits help the CI pass since the secp256k1 subtree directory is touched by the parent PR 24545.
The next 2 commits bring in the mentioned parent PRs. They'll all be removed when the parent PRs get merged.
### commits overview
1. Introducing cryptographic constructs(HKDF, ECDH, ChaCha20, Poly
...
💬 instagibbs commented on pull request "[23.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/26921#issuecomment-1433258574)
Only extra commit/change is af862661654966d5de614755ab9bd1b5913e0959
ACK https://github.com/bitcoin/bitcoin/pull/26921/commits/52376d9217060ce84e992e374d5dc2beae40bb06
(https://github.com/bitcoin/bitcoin/pull/26921#issuecomment-1433258574)
Only extra commit/change is af862661654966d5de614755ab9bd1b5913e0959
ACK https://github.com/bitcoin/bitcoin/pull/26921/commits/52376d9217060ce84e992e374d5dc2beae40bb06
💬 fanquake commented on pull request "Descriptor unit tests and simplifications":
(https://github.com/bitcoin/bitcoin/pull/24361#issuecomment-1433259806)
Drafted for now given it's (maybe) waiting on #26076.
(https://github.com/bitcoin/bitcoin/pull/24361#issuecomment-1433259806)
Drafted for now given it's (maybe) waiting on #26076.
📝 fanquake converted_to_draft a pull request: "Descriptor unit tests and simplifications"
(https://github.com/bitcoin/bitcoin/pull/24361)
Builds on top of #24343.
Adds additional tests, and makes `ToPrivateString()` always succeed, using pubkeys in case privkeys are unavailable.
(https://github.com/bitcoin/bitcoin/pull/24361)
Builds on top of #24343.
Adds additional tests, and makes `ToPrivateString()` always succeed, using pubkeys in case privkeys are unavailable.
💬 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" ?