💬 medobrens83 commented on issue "Bitcoin Kernel Library Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-2814945804)
👍👍🏻💯
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-2814945804)
👍👍🏻💯
💬 l0rinc commented on pull request "ci: Temporarily revert "Drop bench -priority-level in win cross CI"":
(https://github.com/bitcoin/bitcoin/pull/32302#issuecomment-2815018301)
ACK fadccd9e4ab29fa703dd5a6f42fae030176a86f3
Build passes now, we will have to rebase a few PRs after this so it makes sense to merge it ASAP.
(https://github.com/bitcoin/bitcoin/pull/32302#issuecomment-2815018301)
ACK fadccd9e4ab29fa703dd5a6f42fae030176a86f3
Build passes now, we will have to rebase a few PRs after this so it makes sense to merge it ASAP.
💬 l0rinc commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2815039995)
Rebased, cherry picked https://github.com/bitcoin/bitcoin/pull/32302 to fix unrelated CI failure and added back the `unsigned-integer-overflow:CCoinsViewCache` suppressions since it seems like a genuine error that I will have to tackle in a separate PR instead.
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2815039995)
Rebased, cherry picked https://github.com/bitcoin/bitcoin/pull/32302 to fix unrelated CI failure and added back the `unsigned-integer-overflow:CCoinsViewCache` suppressions since it seems like a genuine error that I will have to tackle in a separate PR instead.
💬 fanquake commented on pull request "ci: Temporarily revert "Drop bench -priority-level in win cross CI"":
(https://github.com/bitcoin/bitcoin/pull/32302#issuecomment-2815107502)
Can we add a different change, to continue running all benchmarks (I guess just not Windows), so we aren't back to the same state that allowed #32277 to happen?
(https://github.com/bitcoin/bitcoin/pull/32302#issuecomment-2815107502)
Can we add a different change, to continue running all benchmarks (I guess just not Windows), so we aren't back to the same state that allowed #32277 to happen?
💬 fjahr commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2815218248)
re-ACK e261eb8d50c7192260a449e653453e63f59dbeed
CI failure is unrelated, see #32291
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2815218248)
re-ACK e261eb8d50c7192260a449e653453e63f59dbeed
CI failure is unrelated, see #32291
👍 theStack approved a pull request: "psbt: MuSig2 Fields"
(https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2778346130)
re-ACK e261eb8d50c7192260a449e653453e63f59dbeed
(as per `$ git range-diff debacd1f...e261eb8d`)
Verified that since my previous ACK, the changes didn't affect the logic, and were mostly about code deduplication (for deserializing participant pubkeys), getting rid of magic numbers, variable naming (s/key/pubkey/), and adapting the terminology to the [recent BIP-373 changes](https://github.com/bitcoin/bips/pull/1705) (plain public key -> compressed public key). LGTM.
(https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2778346130)
re-ACK e261eb8d50c7192260a449e653453e63f59dbeed
(as per `$ git range-diff debacd1f...e261eb8d`)
Verified that since my previous ACK, the changes didn't affect the logic, and were mostly about code deduplication (for deserializing participant pubkeys), getting rid of magic numbers, variable naming (s/key/pubkey/), and adapting the terminology to the [recent BIP-373 changes](https://github.com/bitcoin/bips/pull/1705) (plain public key -> compressed public key). LGTM.
💬 theStack commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2050497355)
nit: could add parentheses for better readability (here and below for `PSBT_IN_MUSIG2_PARTIAL_SIG`)
```suggestion
} else if (key.size() != (2 * CPubKey::COMPRESSED_SIZE + 1) && key.size() != (2 * CPubKey::COMPRESSED_SIZE + CSHA256::OUTPUT_SIZE + 1)) {
```
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2050497355)
nit: could add parentheses for better readability (here and below for `PSBT_IN_MUSIG2_PARTIAL_SIG`)
```suggestion
} else if (key.size() != (2 * CPubKey::COMPRESSED_SIZE + 1) && key.size() != (2 * CPubKey::COMPRESSED_SIZE + CSHA256::OUTPUT_SIZE + 1)) {
```
🤔 fjahr reviewed a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2778403152)
Code review ACK acee5c59e68f31acba111bc4d1892b08243ea5e0
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2778403152)
Code review ACK acee5c59e68f31acba111bc4d1892b08243ea5e0
💬 fjahr commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r2050530082)
nit: could make `other` const if you retouch
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r2050530082)
nit: could make `other` const if you retouch
💬 fjahr commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#issuecomment-2815320668)
> LGTM, code review [acee5c5](https://github.com/bitcoin/bitcoin/commit/acee5c59e68f31acba111bc4d1892b08243ea5e0)
Was that an ACK an you just forgot the word or did you just want to say you reviewed? :)
(https://github.com/bitcoin/bitcoin/pull/31243#issuecomment-2815320668)
> LGTM, code review [acee5c5](https://github.com/bitcoin/bitcoin/commit/acee5c59e68f31acba111bc4d1892b08243ea5e0)
Was that an ACK an you just forgot the word or did you just want to say you reviewed? :)
💬 kuegi commented on pull request "[IBD] prevector: allocate `P2WSH`/`P2TR`/`P2PK` scripts on stack":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2050565585)
maybe a "stupid" question, but why not use a constant for the 36 in this file too?
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2050565585)
maybe a "stupid" question, but why not use a constant for the 36 in this file too?
⚠️ SOG-LANG opened an issue: "Secure Bitcoin Source"
(https://github.com/bitcoin/bitcoin/issues/32303)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo
- [x] I still think this issue should be opened here
### Report
### How you generate BTC? Selecting appropriate hardware, installing and configuring mining software, joining a mining pool and setting up a secure wallet. Each component plays an important role in creating an efficient and profitable mining setup.
(https://github.com/bitcoin/bitcoin/issues/32303)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo
- [x] I still think this issue should be opened here
### Report
### How you generate BTC? Selecting appropriate hardware, installing and configuring mining software, joining a mining pool and setting up a secure wallet. Each component plays an important role in creating an efficient and profitable mining setup.
💬 SOG-LANG commented on issue "Secure Bitcoin Source":
(https://github.com/bitcoin/bitcoin/issues/32303#issuecomment-2815414509)

(https://github.com/bitcoin/bitcoin/issues/32303#issuecomment-2815414509)

🤔 l0rinc requested changes to a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2778504085)
As discussed in private, I've summarized my findings here shortly.
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2778504085)
As discussed in private, I've summarized my findings here shortly.
💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050588477)
I know this is meant to be mostly random, but the existing `feature` ones seem to be in alphabetic order
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050588477)
I know this is meant to be mostly random, but the existing `feature` ones seem to be in alphabetic order
💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050585116)
this is a quite confusing way to check whether we're in a child process or not (not obvious to me why `internal_start_stop` is not even here)
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050585116)
this is a quite confusing way to check whether we're in a child process or not (not obvious to me why `internal_start_stop` is not even here)
💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050590886)
this line might need explanation for why no `Traceback` is a `success=False`
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050590886)
this line might need explanation for why no `Traceback` is a `success=False`
💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050590043)
could we add these into the try so that I don't have to understand where `result` is coming from?
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050590043)
could we add these into the try so that I don't have to understand where `result` is coming from?
💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050586381)
do we need to batch all 3 tests into a single test?
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050586381)
do we need to batch all 3 tests into a single test?
💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050594392)
"various startup failures" doesn't sound like a single test to me, could we maybe unwrap them somehow?
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2050594392)
"various startup failures" doesn't sound like a single test to me, could we maybe unwrap them somehow?