💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2077460938)
Sounds like a bug on our end then. The BIP says:

> outpoint (36 bytes): the `COutPoint` of an input (32-byte txid, least significant byte first || 4-byte vout, least significant byte first)

And there's a footnote saying "Why are outpoints little-endian".

https://github.com/bitcoin/bips/blob/57c89ae162b4dab971dc6061ba6acf7d676781ea/bip-0352.mediawiki#cite_ref-why_little_endian_8-0

We should probably have a BIP352 specific `COutPoint` sorter, and a test vector.

👋 fanquake's pull request is ready for review: "depends: swap some cctools tools for LLVM tools"
(https://github.com/bitcoin/bitcoin/pull/29739)

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579633610)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579447042

Personally I think using lowercase for instance methods and uppercase for other methods and functions is good practice because it makes it obvious when you are passing an implicit `this` parameter to function calls. (It is useful for the same reason `m_` prefixes are useful.) However, I got pushback when I tried to allow this in the style guide in #14635 so now I usually try to follow the guide.

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579632793)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579500335

It seems like the goal of the code is to get a new address without unlocking. But I haven't looked into this. It definitely looks a little clumsy, though, because it isn't actually using the error message that is returned.

💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2077491415)
> We should probably have a BIP352 specific `COutPoint` sorter, and a test vector.

Yeah, I'm working on a test vector that incorporates this kind of scenario.

💬 fanquake commented on pull request "depends: build miniupnpc with CMake":
(https://github.com/bitcoin/bitcoin/pull/29707#issuecomment-2077505030)
> FWIW, I found another Windows-specific bug in the upstream: https://github.com/miniupnp/miniupnp/pull/727.

Rebased again, but can pull in this change + fixups on next push

💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1579698526)
done

💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1579698620)
done

💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1579699392)
Thanks for this hint but I think usage of `pathlib` is usually preferred now because it is more modern. So I will keep this as is unless there are more pros to this change that I have overlooked.

💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-2077562279)
Addressed comments from @virtu , thanks for reviewing!

💬 sr-gi commented on pull request "net: Decrease nMaxIPs when learning from DNS seeds":
(https://github.com/bitcoin/bitcoin/pull/29850#issuecomment-2077565724)
Post-merge ACK f2e3662e57eca1330962faf38ff428a564d50a11

💬 twosatsmaxi commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only)":
(https://github.com/bitcoin/bitcoin/pull/29843#issuecomment-2077629808)
Following.

📝 theStack opened a pull request: "refactor: remove remaining unused code from cpp-subprocess"
(https://github.com/bitcoin/bitcoin/pull/29961)
This PR removes remaining code that is unused within the cpp-subprocess module (templates and commented out code). Happy to add more removals if anyone finds more unused parts. Note that there are some API functions of the `Popen` class that we don't use, e.g. `wait()`, `pid()`, `poll()`, `kill()`, but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there.

🤔 tdb3 reviewed a pull request: "contrib: rpcauth.py - Add new option (-json) to output text in json format"
(https://github.com/bitcoin/bitcoin/pull/29433#pullrequestreview-2022987855)
Thanks for squashing commits.
ACK for 9adf949d2aa6d199b85295b18c08967395b5570a

💬 hebasto commented on pull request "refactor: remove remaining unused code from cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/29961#issuecomment-2077685614)
Concept ACK.

💬 achow101 commented on pull request "lint: scripted-diff verification also requires GNU grep":
(https://github.com/bitcoin/bitcoin/pull/29689#issuecomment-2077705790)
ACK 3bf4f8db669e1e274ce2633cf84add2938b9914b

🚀 achow101 merged a pull request: "lint: scripted-diff verification also requires GNU grep"
(https://github.com/bitcoin/bitcoin/pull/29689)

🤔 brunoerg reviewed a pull request: "net: Make AddrFetch connections to fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/26114#pullrequestreview-2023068219)
Just tested with `-nodnsseed -blocksonly -debug=net` with an empty `peers.dat` and checked the behaviour is as expected. Some logs:

1. Adding fixed seeds
```
2024-04-25T13:02:59Z Adding fixed seeds as -dnsseed=0 (or IPv4/IPv6 connections are disabled via -onlynet) and neither -addnode nor -seednode are provided
2024-04-25T13:02:59Z [net] Added hardcoded seed: 1.253.159.19:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 2.152.74.211:8333
2024-04-25T13:02:59Z [net] Added hardcoded se
...

💬 achow101 commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2077749583)
ACK cb8f83df1916308013a6ae4b8e4914eb805c5343

💬 theuni commented on pull request "depends: pass verbose through to cmake based makefiles":
(https://github.com/bitcoin/bitcoin/pull/29960#issuecomment-2077756588)
Concept ACK. I just ran into this locally. I solved it using `V=1 VERBOSE=1`, but this makes sense too.