💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755184675)
I don't think we should be changing all these lines in this commit just for updating the style. It muddies the git blame.

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755179261)
This part `Coins whose output index is 4 (mod 5) have GetCoin() always succeed after being spent` is due to us not erasing these coins with this condition on line 169 below. Since we are not yet not returning spent coins (`// TODO GetCoin shouldn't return spent coins`), we shouldn't remove this line of the comment.

💬 sipa commented on pull request "build: Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344249910)
@ryanofsky I picked the name to match the same thing in [libsecp256k1](https://github.com/bitcoin-core/secp256k1/blob/v0.5.1/CMakePresets.json#L6), but I'm okay with "enable-all" or "all" as well.

💬 maflcko commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1755195956)
Is there a reason to extract those function? I think it would be better to not extract them, because:

* The diff would be smaller
* They are only used and called once
* There shouldn't be a reason for them to be called from somewhere else internally, because `operator<<` is available internally and externally
* If someone wanted to use them externally (for whatever reason), they'd have to be moved again, because they are private right now.

Seems easier to extract them and possibly make
...

🤔 mzumsande reviewed a pull request: "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD"
(https://github.com/bitcoin/bitcoin/pull/30807#pullrequestreview-2298031257)
ACK 992f83bb6f4b29b44f4eaace1d1a2c0001d43cac

> Updated the commit description. Let me know if further updates are needed.

Looks good to me!
> What about us connecting to a synced limited peer, during tip sync, who tries to fetch historical blocks due to the NODE_NETWORK service, like https://github.com/bitcoin/bitcoin/issues/29183.

I think we just wouldn't connect to limited peers during tip sync - only when we are very close to the global tip we accept those as outbound peers (`GetDes
...

💬 achow101 commented on pull request "docs: updated developer notes for --with-sanitizers to -DSANITIZERS":
(https://github.com/bitcoin/bitcoin/pull/30870#issuecomment-2344256749)
ACK 4b1ce3cac81497dd094e8c34550edf633f4d38ae

🚀 achow101 merged a pull request: "docs: updated developer notes for --with-sanitizers to -DSANITIZERS"
(https://github.com/bitcoin/bitcoin/pull/30870)

💬 achow101 commented on pull request "docs: Updated debug build instructions for cmake":
(https://github.com/bitcoin/bitcoin/pull/30840#issuecomment-2344260817)
ACK 0b003e1ff7e09b56cd013b15f1998495994b7211

TIL `ccmake`.

🚀 achow101 merged a pull request: "docs: Updated debug build instructions for cmake"
(https://github.com/bitcoin/bitcoin/pull/30840)

💬 achow101 commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#issuecomment-2344266604)
ACK 992f83bb6f4b29b44f4eaace1d1a2c0001d43cac

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755201921)
[Done](https://github.com/bitcoin/bitcoin/compare/f68e9d8887651828bd99635165fcec43f5cf6d4a..376ba5fcfd72a253b2dfe853781a10eee1af2ccb), thanks

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755207524)
Fair, [reverted](https://github.com/bitcoin/bitcoin/compare/f68e9d8887651828bd99635165fcec43f5cf6d4a..376ba5fcfd72a253b2dfe853781a10eee1af2ccb) the unrelated lines

💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1755219785)
Not reuse, but to obviate that the method does two separate things: complex logic for how to serialize a number, followed by inserting bytes - so basically a separation of concerns.

And also https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741078658 indicated that operators may not be here to stay, so it makes sense to unburden them early.

🚀 achow101 merged a pull request: "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD"
(https://github.com/bitcoin/bitcoin/pull/30807)

👋 achow101's pull request is ready for review: "[28.x] Further backports and rc2"
(https://github.com/bitcoin/bitcoin/pull/30827)

💬 achow101 commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#issuecomment-2344308159)
Backported to 28.x in #30827

👍 tdb3 approved a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2129278263)
ACK 69bf58dc0e25897e9fde435c9823a921590a90dc

This is definitely an improvement on the existing loadwallet help.

nit: I believe the RPC page listed in the commit message is actually derived from the help output of `loadwallet`. Technically this PR addresses the help output, and downstream things that use it would also benefit. I don't really see a crucial need to change the commit message though, so feel free to ignore. If you happen to change something else, it might be good to also upd
...

💬 tdb3 commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646864324)
nit: Same for here:
`Wallet with files under wallets/foo/specialWallet/:`

💬 tdb3 commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646863530)
nit: Stating `Load regular` seems extraneous here (and usage of `regular` might bring up some other questions (e.g. non-regular?)). Shortening to `Wallet with files under wallets/foo/:` seems ok.

💬 tdb3 commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646865129)
nit: Same for here:
`wallet specified by absolute path`