💬 fjahr commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1981926993)
``` suggestion
# txid + coins per txid + vout + coin code (height*2 + coinbase)
```

💬 fjahr commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1981855608)
nit-ish: Reference is not fully correct

``` suggestion
# test cases from serialize_tests.cpp:varints_bitpatterns
```

💬 darosior commented on pull request "qa: clarify and document one assumeutxo test case with malleated snapshot":
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2701746604)
Thanks for the reviews. Updated title and description to only mention one of the test cases since i dropped the other commit. I don't think it's worth addressing the nits unless people feel too strongly.

💬 achow101 commented on pull request "doc: remove note about macOS self-signing":
(https://github.com/bitcoin/bitcoin/pull/32003#issuecomment-2701770015)
It may still be useful to mention `xattr -d com.apple.quarantine` for the binaries since it's necessary to do that to run them from finder. Although I'm not sure it even makes sense to try to run our binaries from finder. Even with `bitcoin-qt`, it still opens up a terminal that has to be open while the GUI is running.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2701794581)
@ryanofsky thanks. I'm probably going to leave that to followup, to preserve ACKs and limit the scope of this PR.

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1982076973)
Fixed

💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2701888252)
Added a commit that should fix the fuzz failure.

💬 fjahr commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1982113087)
Understood. I think I would have chosen a different name for the `ConstPubkeyProvider`, like `GetConstPrivKey` maybe, but completely optional.

💬 fjahr commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#issuecomment-2701945658)
utACK 1bfabb4f4ec07c8fb0dfc2515d413b5196e01348

💬 hodlinator commented on pull request "RFC: Add `operator""_uint256` compile-time user-defined literal":
(https://github.com/bitcoin/bitcoin/pull/31991#discussion_r1982117901)
Thanks for trying that out!
I like that the reversal is more directly tied to the literal than when you were using `_hex`.

Hm... now that I compare:
```
"000000000000000000010b17283c3c400507969a9c2afd1dcf2082ec5cca2880"_uint256
```
and
```
uint256{"000000000000000000010b17283c3c400507969a9c2afd1dcf2082ec5cca2880"}
```
...does one character less really justify adding a literal?

Could shorten it to `_u256`, but that has a different meaning than `u8` in `_hex_u8` (one single unsigned
...

💬 l0rinc commented on pull request "RFC: Add `operator""_uint256` compile-time user-defined literal":
(https://github.com/bitcoin/bitcoin/pull/31991#discussion_r1982128012)
> does one character less really justify adding a literal?

I'm not counting chars, but that the noise is at the end now, it starts with the value.
I.e. that we see `.blockhash = "000000000000000000010b17283c3c400507969a9c2afd1dcf2082ec5cca2880"...` directly now.

💬 fjahr commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2701970565)
utACK 7edaf8b64cb2d59ada22042fee62a417e52368b8

💬 TheCharlatan commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#discussion_r1982137766)
Are the TODOs going to be addressed here?

💬 mzumsande commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1982140636)
this is changed back and fourth between the two commits, probably a rebase error.

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