💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3102421425)
> So in that case I think the bevavior should be:
>
> * if coins are sent to a bech32m address, use a silent payment change address (don't use the regular bech32m descriptor)
> * otherwise, use a matching output type
>
> * when there's no descriptor for it, fall back to silent payment change address

Done

💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2222306224)
> You appear to be talking about running a test with two different UTXO sets generated from the same set of effective values at different feerates. The latter is done already in the test I linked above. For the former the invariant you describe does not hold.

Yes, that is what I'm talking about said a different way. Yes, the test you linked above tests for that but only for the same static values.

💬 kevkevinpal commented on pull request "test: functional test for incomplete PSBT with additional signatures required":
(https://github.com/bitcoin/bitcoin/pull/33035#issuecomment-3102434281)
I tried running the test and generating a coverage report but did not see any additional coverage, here is how I did it

---

[doc to build coverage report](https://github.com/bitcoin/bitcoin/blob/7129c9ea8e950f50bdc56d88c57617c66c90bb8a/doc/developer-notes.md#using-llvmclang-toolchain)

Below is the direct commands to generate the report for this test `rpc_psbt.py`
```
cmake -B build -DCMAKE_C_COMPILER="clang" \
-DCMAKE_CXX_COMPILER="clang++" \
-DAPPEND_CFLAGS="-fprofile-instr-g
...

💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2222316261)
> The test you propose is also interesting, and someone might want to take a stab at adding it in another pull request.

That would be great.

💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2222321444)
> UTXOs are ordered in descending effective value by BnB, so I don’t think the UTXO with largest value being likely to be generated first is a problem.

I was thinking of testing this more as you would a black box.

👍 maflcko approved a pull request: "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline"
(https://github.com/bitcoin/bitcoin/pull/32279#pullrequestreview-3042653949)
I haven't tested IBD/reindex performance.

review ACK f7d9f4510d537b280808e1e8e203c9445c8ad4df 🍴

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
truste
...

💬 maflcko commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2222294318)
nit in 3a9c80f7b4807e9abd9f2da36e8ee479ecf00d19:

`GetPubKey` does not need to be wrapped in `CPubKey{}`. You could just store a `dummy_pubkey` for all test cases to re-use.

👍 theStack approved a pull request: "Update secp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/33036#pullrequestreview-3042764955)
ACK 336b8be37b2260d8e902b93f1761265a0aefa496

🤔 danielabrozzoni reviewed a pull request: "p2p: rename GetAddresses -> GetAddressesUnsafe"
(https://github.com/bitcoin/bitcoin/pull/32994#pullrequestreview-3042695024)
last push (1cb23997033c395d9ecd7bf2f54787b134485f41):
- rename `GetAddressesUncached` -> `GetAddressesUnsafe`
- improve documentation

💬 danielabrozzoni commented on pull request "p2p: rename GetAddresses -> GetAddressesUnsafe":
(https://github.com/bitcoin/bitcoin/pull/32994#discussion_r2222322327)
Fair enough, I think `Uncached` scares whoever knows about the getaddr cache, but if you don't have that context it's not immediately obvious.

I changed it to `GetAddresses`/`GetAddressesUnsafe` :)

💬 danielabrozzoni commented on pull request "p2p: rename GetAddresses -> GetAddressesUnsafe":
(https://github.com/bitcoin/bitcoin/pull/32994#discussion_r2222349382)
This is nice, thank you! I have included it and added you as a co-author of the last commit

💬 darosior commented on pull request "doc: Add release notes for 32521 (MAX_TX_LEGACY_SIGOPS)":
(https://github.com/bitcoin/bitcoin/pull/33037#issuecomment-3102529838)
Ack faa2f3b1afe706c6d44fa1949dd97c9e6df5c9d1

Thank you for taking care of that.

-------- Original Message --------
On 7/22/25 7:24 AM, Sjors Provoost wrote:

> Sjors left a comment [(bitcoin/bitcoin#33037)](https://github.com/bitcoin/bitcoin/pull/33037#issuecomment-3102303178)
>
> ACK [faa2f3b](https://github.com/bitcoin/bitcoin/commit/faa2f3b1afe706c6d44fa1949dd97c9e6df5c9d1)
>
> —
> Reply to this email directly, [view it on GitHub](https://github.com/bitcoin/bitcoin/pull/33037#issuecomment-
...

💬 darosior commented on pull request "[29.x] Backport #32521":
(https://github.com/bitcoin/bitcoin/pull/33013#issuecomment-3102584840)
Added a commit with the suggested release note. While i was there i also changed my nym for my name in the credits.

💬 brunoerg commented on pull request "test: check tx is final when there is no locktime":
(https://github.com/bitcoin/bitcoin/pull/33030#issuecomment-3102631463)
> Makes sense. utACK [7180b82](https://github.com/bitcoin/bitcoin/commit/7180b82420c0f303140a93ca635f5ac806bea77d). Could add a comment explaining this to the test as it may not be immediately obvious to a reader.

Thank you, I just added a comment for this.

💬 maflcko commented on pull request "test: check tx is final when there is no locktime":
(https://github.com/bitcoin/bitcoin/pull/33030#issuecomment-3102678470)
lgtm ACK 065e42976a70738770af256da810ddc1316a4496

👍 darosior approved a pull request: "test: check tx is final when there is no locktime"
(https://github.com/bitcoin/bitcoin/pull/33030#pullrequestreview-3042985591)
utACK 065e42976a70738770af256da810ddc1316a4496

💬 darosior commented on pull request "test: check tx is final when there is no locktime":
(https://github.com/bitcoin/bitcoin/pull/33030#discussion_r2222521762)
nit, don't retouch this for me, but my point with the comment wasn't to describe the line below (anyone can just read block height is 0 and locktime is 0) but to describe *why* them both being 0 is a special case worth exercising, as you did in OP.

💬 naiyoma commented on pull request "test: add test cases to wallet_signer.py":
(https://github.com/bitcoin/bitcoin/pull/33020#discussion_r2222554679)
done https://github.com/bitcoin/bitcoin/commit/38a6293fa89104f3d6f1907d04c2e4c488157307

💬 naiyoma commented on pull request "test: add test cases to wallet_signer.py":
(https://github.com/bitcoin/bitcoin/pull/33020#issuecomment-3102747234)

> For the third commit [74fb47f](https://github.com/bitcoin/bitcoin/commit/74fb47f071451145b550062e382416db9388433d): I suggest deleting the commented out code. It's not doing anything useful for the test, and I can't remember why I wrote it.

@Sjors, thanks for the review.
I have also deleted this part https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_signer.py#L235

💬 Sjors commented on pull request "test: add test cases to wallet_signer.py":
(https://github.com/bitcoin/bitcoin/pull/33020#issuecomment-3102748924)
ACK da318fe53fa954cc3eadd7c39e17eb3da7c3e09e