💬 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.
```
💬 jonatack commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1450905336)
Perhaps add a "see `AddSingle()`" mention somewhere in here, as the logic referred to here is in that method called from this one (`Add/Add_`).
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1450905336)
Perhaps add a "see `AddSingle()`" mention somewhere in here, as the logic referred to here is in that method called from this one (`Add/Add_`).
💬 jonatack commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1450951378)
```suggestion
// Adding service flags even works when the addr is in Tried; see `AddSingle()`
```
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1450951378)
```suggestion
// Adding service flags even works when the addr is in Tried; see `AddSingle()`
```
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1890013810)
Gawd, SFFO is such pain in the neck.
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1890013810)
Gawd, SFFO is such pain in the neck.
💬 ryanofsky commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1450962291)
> Again, no opinion on this. I think either solution is fine. PRs welcome, I guess?
Thanks, I started playing around with a change that should make it easier to set the category without being verbose, and also start relying less on a global logging instance, which should be useful for kernel applications and tests: 262923c5b37f62a925a2c5611318855009bfb241. Will see how this looks and probably open a PR.
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1450962291)
> Again, no opinion on this. I think either solution is fine. PRs welcome, I guess?
Thanks, I started playing around with a change that should make it easier to set the category without being verbose, and also start relying less on a global logging instance, which should be useful for kernel applications and tests: 262923c5b37f62a925a2c5611318855009bfb241. Will see how this looks and probably open a PR.