💬 jonatack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1450864532)
9eed22e nit, code unneeded until/unless there is a `grant`
```diff
@@ -2316,12 +2316,12 @@ void CConnman::ProcessAddrFetch()
strDest = m_addr_fetches.front();
m_addr_fetches.pop_front();
}
- // Attempt v2 connection if we support v2 - we'll reconnect with v1 if our
- // peer doesn't support it or immediately disconnects us for another reason.
- const bool use_v2transport(GetLocalServices() & NODE_P2P_V2);
- CAddress addr;
CSemaphoreGrant grant(*
...
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1450864532)
9eed22e nit, code unneeded until/unless there is a `grant`
```diff
@@ -2316,12 +2316,12 @@ void CConnman::ProcessAddrFetch()
strDest = m_addr_fetches.front();
m_addr_fetches.pop_front();
}
- // Attempt v2 connection if we support v2 - we'll reconnect with v1 if our
- // peer doesn't support it or immediately disconnects us for another reason.
- const bool use_v2transport(GetLocalServices() & NODE_P2P_V2);
- CAddress addr;
CSemaphoreGrant grant(*
...
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1889878218)
I’ve added another fuzz target that validates the output of CoinGrinder against a bruteforce search for the smallest weight input set.
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1889878218)
I’ve added another fuzz target that validates the output of CoinGrinder against a bruteforce search for the smallest weight input set.
👍 jamesob approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1818940398)
ACK d63b2e88780dc78fd531b053653361a0bf3fcbea ([`jamesob/ackr/28960.1.TheCharlatan.kernel_remove_dependency`](https://github.com/jamesob/bitcoin/tree/ackr/28960.1.TheCharlatan.kernel_remove_dependency))
Reviewed and ran tests locally. Changes are pretty straightforward and common-sense.
Putting event queue execution into the hands of libbitcoinkernel users seems like the right design choice, and it is especially motivating that this change helps novel fuzz testing (#29158).
As an aside,
...
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1818940398)
ACK d63b2e88780dc78fd531b053653361a0bf3fcbea ([`jamesob/ackr/28960.1.TheCharlatan.kernel_remove_dependency`](https://github.com/jamesob/bitcoin/tree/ackr/28960.1.TheCharlatan.kernel_remove_dependency))
Reviewed and ran tests locally. Changes are pretty straightforward and common-sense.
Putting event queue execution into the hands of libbitcoinkernel users seems like the right design choice, and it is especially motivating that this change helps novel fuzz testing (#29158).
As an aside,
...
💬 jamesob commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1450864735)
https://github.com/bitcoin/bitcoin/pull/28960/commits/d63b2e88780dc78fd531b053653361a0bf3fcbea
I guess this periodic call was originally included to demonstrate how you might use the scheduler from the libbitcoinkernel interface, but now that we've removed the scheduler from the interface there isn't really an equivalent. Clients would be expected to run their own async event managers and periodic schedulers, which makes sense - and I guess is the whole point of this change!
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1450864735)
https://github.com/bitcoin/bitcoin/pull/28960/commits/d63b2e88780dc78fd531b053653361a0bf3fcbea
I guess this periodic call was originally included to demonstrate how you might use the scheduler from the libbitcoinkernel interface, but now that we've removed the scheduler from the interface there isn't really an equivalent. Clients would be expected to run their own async event managers and periodic schedulers, which makes sense - and I guess is the whole point of this change!
💬 kristapsk commented on issue "rpc: actually deprecate `rpcuser` & `rpcpass`":
(https://github.com/bitcoin/bitcoin/issues/29240#issuecomment-1889887704)
> If we are going to deprecate these options, now seems like a good time to do so. We could start with updating all exmaples / docs in this repository, to remove any `rpcuser`/`rpcpass` usage.
That should definitely be done first.
One thing with cookie auth, that currently needs hacks, is allowing cookie access for other users. #28167
(https://github.com/bitcoin/bitcoin/issues/29240#issuecomment-1889887704)
> If we are going to deprecate these options, now seems like a good time to do so. We could start with updating all exmaples / docs in this repository, to remove any `rpcuser`/`rpcpass` usage.
That should definitely be done first.
One thing with cookie auth, that currently needs hacks, is allowing cookie access for other users. #28167
💬 fjahr commented on pull request "During IBD, prune as much as possible until we get close to where we will eventually keep blocks":
(https://github.com/bitcoin/bitcoin/pull/20827#issuecomment-1889900815)
utACK d298ff8b62b2624ed390c8a2f905c888ffc956ff
While there may be slightly better configurations possible, as @0xB10C pointed out above, I think the 1MB assumption is alright and this is a clear improvement already, so this can be merged and potentially improved later.
(https://github.com/bitcoin/bitcoin/pull/20827#issuecomment-1889900815)
utACK d298ff8b62b2624ed390c8a2f905c888ffc956ff
While there may be slightly better configurations possible, as @0xB10C pointed out above, I think the 1MB assumption is alright and this is a clear improvement already, so this can be merged and potentially improved later.
🤔 sr-gi reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1816369526)
First pass, untested, will do a more thorough review next week
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1816369526)
First pass, untested, will do a more thorough review next week
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449320797)
nit: feels like this is something that you could do on `__init__()` instead of creating an empty array. It's not a big deal though, so feel free to disregard
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449320797)
nit: feels like this is something that you could do on `__init__()` instead of creating an empty array. It's not a big deal though, so feel free to disregard
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449286884)
nit: queued / queue of messages / messages queued
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449286884)
nit: queued / queue of messages / messages queued
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449289058)
I think this field is redundant. This will always be `True` if `v2_state` is set, and `False` otherwise, so we could drop it and just check against whether v2_state is `None`.
If you think that's too verbose, you could also create a helper method:
```python
def supports_v2_p2p(self):
return self.v2_state is not None
```
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449289058)
I think this field is redundant. This will always be `True` if `v2_state` is set, and `False` otherwise, so we could drop it and just check against whether v2_state is `None`.
If you think that's too verbose, you could also create a helper method:
```python
def supports_v2_p2p(self):
return self.v2_state is not None
```
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449336950)
nit: space missing after responder, also I guess add "the" before to match the previous suggestion if you happen to take it
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449336950)
nit: space missing after responder, also I guess add "the" before to match the previous suggestion if you happen to take it
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449352220)
Is this necessary?
`self.v2_state.received_prefix` is initialized to `b""`, whose length is zero. Therefore, and given you are only using this as offset for `self.recvbuf`, I think you could just dump the variable and do (in line 266):
```
response = self.v2_state.complete_handshake(BytesIO(self.recvbuf[len(self.v2_state.received_prefix):]))
```
Or define the variable if you want a shorter call but just right before using it, without having to set it to zero first.
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449352220)
Is this necessary?
`self.v2_state.received_prefix` is initialized to `b""`, whose length is zero. Therefore, and given you are only using this as offset for `self.recvbuf`, I think you could just dump the variable and do (in line 266):
```
response = self.v2_state.complete_handshake(BytesIO(self.recvbuf[len(self.v2_state.received_prefix):]))
```
Or define the variable if you want a shorter call but just right before using it, without having to set it to zero first.
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1450888252)
nit: connection (for consistency with the rest)
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1450888252)
nit: connection (for consistency with the rest)
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1450866270)
I'm really struggling to understand what is going on here based on these comments (and the ones bellow at L98-99).
To my understanding, what we are trying to test here is: given a peer of `node0` (`peer6`) that provides some blocks to it (decoy on the first iteration and regular on the second) if after connecting `node0` and `node1`, they do end up sharing the same tip. However, I feel the comments are hard to understand
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1450866270)
I'm really struggling to understand what is going on here based on these comments (and the ones bellow at L98-99).
To my understanding, what we are trying to test here is: given a peer of `node0` (`peer6`) that provides some blocks to it (decoy on the first iteration and regular on the second) if after connecting `node0` and `node1`, they do end up sharing the same tip. However, I feel the comments are hard to understand
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449337692)
nit: P2PConnection is "the" initiator?
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449337692)
nit: P2PConnection is "the" initiator?
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449340059)
nit: caps in "the"
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449340059)
nit: caps in "the"
💬 chrisguida commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889953978)
Concept ACK
This PR seems to be the minimal feature set to get:
- LN-Symmetry,
- much better PTLCs, and
- all of the powerful UTXO-sharing protocols that CTV enables
Regarding LN-Symmetry, this update will FINALLY enable symmetric channel updates and eliminate the risk of losing all your money due to restoring an old backup. It also makes on-chain resolution in the case of uncooperative channel closes much less painful, and makes watchtowers much more efficient. There are still some mem
...
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889953978)
Concept ACK
This PR seems to be the minimal feature set to get:
- LN-Symmetry,
- much better PTLCs, and
- all of the powerful UTXO-sharing protocols that CTV enables
Regarding LN-Symmetry, this update will FINALLY enable symmetric channel updates and eliminate the risk of losing all your money due to restoring an old backup. It also makes on-chain resolution in the case of uncooperative channel closes much less painful, and makes watchtowers much more efficient. There are still some mem
...
💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1889982713)
ready for review
cc @glozow @ismaelsadeeq @achow101
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1889982713)
ready for review
cc @glozow @ismaelsadeeq @achow101
🤔 jonatack reviewed a pull request: "doc, test: test and explain service flag handling"
(https://github.com/bitcoin/bitcoin/pull/29213#pullrequestreview-1819009947)
ACK d536e5a6325d1885224f36debdcc5245b94efe8a
(https://github.com/bitcoin/bitcoin/pull/29213#pullrequestreview-1819009947)
ACK d536e5a6325d1885224f36debdcc5245b94efe8a
💬 jonatack commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1450936649)
It seems to me this might be helpful and the message names are unlikely to change.
```diff
// Overwrites potentially existing services. In contrast to this,
- // unvalidated services received via gossip relay are only
- // ever added but cannot replace existing ones.
+ // unvalidated services received via gossip relay in ADDR/ADDRV2
+ // messages are only ever added but do not replace existing ones.
```
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1450936649)
It seems to me this might be helpful and the message names are unlikely to change.
```diff
// Overwrites potentially existing services. In contrast to this,
- // unvalidated services received via gossip relay are only
- // ever added but cannot replace existing ones.
+ // unvalidated services received via gossip relay in ADDR/ADDRV2
+ // messages are only ever added but do not replace existing ones.
```