💬 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.
(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.
(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
...
(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
...
(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
(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)
(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`.
(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)
(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
(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
(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
(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.
(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)
(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)
(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
(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
...
(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/:`
(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.
(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`
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646865129)
nit: Same for here:
`wallet specified by absolute path`
💬 tdb3 commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1755238118)
This seems to discuss passing in the absolute path of the .dat file. Saw the following behavior:
(regtest=1 in bitcoin.conf)
```
dev@bdev01:~/bitcoin$ src/bitcoind -daemonwait
Bitcoin Core starting
dev@bdev01:~/bitcoin$ src/bitcoin-cli createwallet foo
{
"name": "foo"
}
dev@bdev01:~/bitcoin$ src/bitcoin-cli stop
Bitcoin Core stopping
dev@bdev01:~/bitcoin$ src/bitcoind -daemonwait
Bitcoin Core starting
dev@bdev01:~/bitcoin$ ls -l /home/dev/.bitcoin/regtest/wallets/foo/wallet.dat
...
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1755238118)
This seems to discuss passing in the absolute path of the .dat file. Saw the following behavior:
(regtest=1 in bitcoin.conf)
```
dev@bdev01:~/bitcoin$ src/bitcoind -daemonwait
Bitcoin Core starting
dev@bdev01:~/bitcoin$ src/bitcoin-cli createwallet foo
{
"name": "foo"
}
dev@bdev01:~/bitcoin$ src/bitcoin-cli stop
Bitcoin Core stopping
dev@bdev01:~/bitcoin$ src/bitcoind -daemonwait
Bitcoin Core starting
dev@bdev01:~/bitcoin$ ls -l /home/dev/.bitcoin/regtest/wallets/foo/wallet.dat
...