💬 mzumsande commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1982155510)
why is the check changed from `encoded_string.empty()` to `decoded.empty()` in the first commit - especially if this function is being rewritten in the next commit anyway?

💬 theuni commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2702063374)
Sorry for waiting to late to chime in on this, but I'm afraid I'm a concept NACK. We discussed this at length at CoreDev, and I'll briefly try to sum up my thoughts:

- I believe this is the wrong abstraction level. SockMan is essentially mimicking the behavior of a generic io multiplexing framework (libevent, libuv, etc.), but without the feature-set of those libs.
- The internals and api are heavily biased on Core's p2p layer, and aren't well-suited for abstracting. In particular, its event
...

💬 l0rinc commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1982213549)
Thanks for the review. It was a rebase error indeed, thanks for catching it.

💬 l0rinc commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1982213594)
it's the decoded size that determines if the `0, decoded.size() - 1` interval makes sense or not - I'm correcting it in the first commit since this was a bug that could cause a fuzz failure (added explanation to commit message).
I'm reordering in the second commit so that we have single roundtrip logic instead of 2 separate ones.

🤔 mzumsande reviewed a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2662599068)
Code Review ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda

💬 jamesob commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#discussion_r1982268153)
In this case, likely not - Jeremy was initially thinking that if there was some way we could rule out use of OP_CTV in the script about to be evaluated, we could forgo warming the caches below. But (aside from that being probably impossible) this TODO was made [prior to benchmarking](https://github.com/bitcoin/bitcoin/pull/21702#issuecomment-1026922813) that shows there isn't any detectable slowdown to initializing those caches.

I'll remove the comment.

🤔 hebasto reviewed a pull request: "doc: add note to Windows build about stripping bins"
(https://github.com/bitcoin/bitcoin/pull/32002#pullrequestreview-2662658024)
Concept ACK on amending documentation.

I believe, that the [`--strip`](https://cmake.org/cmake/help/v3.22/manual/cmake.1.html#install-a-project) option is what we need:
```
cmake --install build --prefix /mnt/c/workspace/bitcoin --strip
```

💬 hebasto commented on pull request "doc: add note to Windows build about stripping bins":
(https://github.com/bitcoin/bitcoin/pull/32002#discussion_r1982284955)
The `--target` option is not available for the `cmake --install` command:
```
$ cmake --install build --target install/strip
Unknown argument --target
...
```

💬 moonsettler commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#discussion_r1982287214)
Shouldn't it be set to `force` instead of `true`?

Makes this if completely superfluous:
```c++
if (uses_bip143_segwit || uses_bip341_taproot || uses_bip119_ctv) {
// Computations shared between both sighash schemes.
m_prevouts_single_hash = GetPrevoutsSHA256(txTo);
m_sequences_single_hash = GetSequencesSHA256(txTo);
m_outputs_single_hash = GetOutputsSHA256(txTo);

// 0 hash used to signal if we should skip scriptSigs
// when re-co
...

💬 jamesob commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#discussion_r1982294915)
@moonsettler no - `force` is simply the initial value for the segwit/taproot detection variables. Since there isn't a similar detection process for CTV, we must assume its cache prep is always required. But that's a good point that it's probably worthwhile to strip out the `if`.

Likely the only way we could make `use_bip119_ctv` actually variable is on the basis of activation height. Hence the original TODO.

💬 ariard commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2702260349)
> This change can be composed with other opcode specifications (e.g. [CSFS](https://bitcoinops.org/en/topics/op_checksigfromstack/),

@jamesob see my pending email in answer to AJ for the issues with CSFS building on @naumenkogs’s TxWithhold:
- https://blog.bitmex.com/txwithhold-smart-contracts/
- https://gnusha.org/pi/bitcoindev/f594c2f8-d712-48e4-a010-778dd4d0cadb@Spark/

Especially if it’s possible to do a TxWithhold on the spend of any coinbase output beyond `COINBASE_MATURITY`. At lea
...

💬 moonsettler commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#discussion_r1982310351)
> Likely the only way we could make use_bip119_ctv actually variable is on the basis of activation height. Hence the original TODO.

Sounds better than nothing.

📝 hebasto opened a pull request: "qt: 29.0 translations update"
(https://github.com/bitcoin/bitcoin/pull/32004)
This PR follows our [Release Process](https://github.com/bitcoin/bitcoin/blob/bd0ee07310c3dcdd08633c69eac330e2e567b235/doc/release-process.md) and concludes the translation-specific efforts for this release cycle. It follows two previous translation-related PRs, https://github.com/bitcoin/bitcoin/pull/31809 and https://github.com/bitcoin-core/gui/pull/854.

It is one of the steps required _before_ branch-off, as scheduled in https://github.com/bitcoin/bitcoin/issues/31029.

The previous simi
...

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2702303949)
cc @stickies-v @pablomartin4btc @johnny9 @jarolrod as regular reviewers of similar previous PRs.

cc @glozow as the 29.0 release manager.

📝 Hosampor opened a pull request: "Create cmake-multi-platform.yml"
(https://github.com/bitcoin/bitcoin/pull/32005)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ achow101 closed a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/32005)

📝 Hosampor opened a pull request: "Create cmake-single-platform.yml"
(https://github.com/bitcoin-core/gui/pull/856)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ achow101 closed a pull request: "."
(https://github.com/bitcoin-core/gui/pull/856)

💬 pinheadmz commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2702330203)
@ariard I think your comment is more appropriate on the mailing list or delving. You are even mentioning a specific post from the mailing list. I expect a lot of off-topic comments on this PR and would appreciate your help staying focused. PR comments MUST be entirely focused on THIS PR code.

👍 luke-jr approved a pull request: "doc: remove note about macOS self-signing"
(https://github.com/bitcoin/bitcoin/pull/32003#pullrequestreview-2662750676)
ACK, confirmed this is no longer necessary on macOS