💬 AngusP commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605892564)
Worth adding a note to the comment?
```python
# Use subversion as peer id. Test nodes have their node number appended to the UA
```
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605892564)
Worth adding a note to the comment?
```python
# Use subversion as peer id. Test nodes have their node number appended to the UA
```
💬 AngusP commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605893483)
This comment now doesn't really make sense to me?
Maybe
```suggestion
# wait until peers have completed a version handshake and so match the
# expected connection subversion
```
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605893483)
This comment now doesn't really make sense to me?
Maybe
```suggestion
# wait until peers have completed a version handshake and so match the
# expected connection subversion
```
💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605903225)
> This comment now doesn't really make sense to me?
Why? the procedure behavior is the same.
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605903225)
> This comment now doesn't really make sense to me?
Why? the procedure behavior is the same.
💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605903301)
> Worth adding a note to the comment?
>
> ```python
> # Use subversion as peer id. Test nodes have their node number appended to the UA
> ```
sure. Pushed.
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605903301)
> Worth adding a note to the comment?
>
> ```python
> # Use subversion as peer id. Test nodes have their node number appended to the UA
> ```
sure. Pushed.
💬 rustyrussell commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1605921836)
Sure, wasn't sure this was the place, but is there somewhere better? Seems like all the regulars "know" this, so putting it where a beginner might look is helpful.
Was thinking the version info is useful for backporting, but that's a limited window and is already in the commit message if people want.
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1605921836)
Sure, wasn't sure this was the place, but is there somewhere better? Seems like all the regulars "know" this, so putting it where a beginner might look is helpful.
Was thinking the version info is useful for backporting, but that's a limited window and is already in the commit message if people want.
💬 edilmedeiros commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1605924276)
I think adding this info here as you propose with the wording suggested by @laanwj would have some chance to be merged (I would ack it). Just don't expect it to attract much attention of reviewers since lacking this info doesn't seem to create much problem.
About backporting, even small nits of documentation are somewhat complex and is reserved to be done by a few maintainers from time to time. I don't believe it will happen with this since this is not critical and doesn't apply to earlier vers
...
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1605924276)
I think adding this info here as you propose with the wording suggested by @laanwj would have some chance to be merged (I would ack it). Just don't expect it to attract much attention of reviewers since lacking this info doesn't seem to create much problem.
About backporting, even small nits of documentation are somewhat complex and is reserved to be done by a few maintainers from time to time. I don't believe it will happen with this since this is not critical and doesn't apply to earlier vers
...
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2119145090)
Squashed all fixme's into their logical commit. This should be in a good state for review now. The only TODO that is left to do is the Qt settings migration, i could use some help with that because i don't really know the new mechanism.
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2119145090)
Squashed all fixme's into their logical commit. This should be in a good state for review now. The only TODO that is left to do is the Qt settings migration, i could use some help with that because i don't really know the new mechanism.
💬 GregTonoski commented on pull request "Policy: Enforce witness script size limit for tapscript":
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2119182536)
Could @glozow summarize cons, please? I couldn't find any in spite of huge amount of text in the comments. The PR does not introduce a "breaking change" contrary to what is claimed by opponents.
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2119182536)
Could @glozow summarize cons, please? I couldn't find any in spite of huge amount of text in the comments. The PR does not introduce a "breaking change" contrary to what is claimed by opponents.
👍 AngusP approved a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118#pullrequestreview-2065044318)
tACK f4c588c98f163e9fa9083821ba36e1053f8c1496
(https://github.com/bitcoin/bitcoin/pull/30118#pullrequestreview-2065044318)
tACK f4c588c98f163e9fa9083821ba36e1053f8c1496
💬 AngusP commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1606000057)
I think just the way it read with the old impl vs the new, "waiting until we find a peer connection matching some predicate" rather than "wait until the peer has a non-zero version" . But on second thoughts you're right it's not functionally different so disregard my suggestion
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1606000057)
I think just the way it read with the old impl vs the new, "waiting until we find a peer connection matching some predicate" rather than "wait until the peer has a non-zero version" . But on second thoughts you're right it's not functionally different so disregard my suggestion
💬 marcofleon commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2119185471)
Adjusted some things on my machine and used `-merge=1` and it successfully merges after several attempts. Thank you for the help @maflcko
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2119185471)
Adjusted some things on my machine and used `-merge=1` and it successfully merges after several attempts. Thank you for the help @maflcko
💬 Psifour commented on pull request "Policy: Enforce witness script size limit for tapscript":
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2119197600)
You are asking the wrong question, instead it should be framed as "what technical threat is posed and is said threat sufficient to warrant the creation of a new standardness rule?" The answers to those questions are "none" and subsequently "no".
Applying rules meant to solve a specific problem to systems that can't/don't exhibit the same vulnerabilities would be wildly inappropriate. It would be similar to applying the bare multi-sig limit (3) to ALL multisigs.
TL;DR: The PR attempts to so
...
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2119197600)
You are asking the wrong question, instead it should be framed as "what technical threat is posed and is said threat sufficient to warrant the creation of a new standardness rule?" The answers to those questions are "none" and subsequently "no".
Applying rules meant to solve a specific problem to systems that can't/don't exhibit the same vulnerabilities would be wildly inappropriate. It would be similar to applying the bare multi-sig limit (3) to ALL multisigs.
TL;DR: The PR attempts to so
...
💬 GregTonoski commented on pull request "Policy: Enforce witness script size limit for tapscript":
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2119239759)
> ... policy rule in earlier Script version environments is to discourage constructs that suffer from DoS concerns in the interpreter (known and unknown) (...) It is of course possible there are oversights...
Can I ask to point to test results of the change introduced in TapRoot (relaxation of the 3600 bytes script size standard limit/mempool policy rule), please?
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2119239759)
> ... policy rule in earlier Script version environments is to discourage constructs that suffer from DoS concerns in the interpreter (known and unknown) (...) It is of course possible there are oversights...
Can I ask to point to test results of the change introduced in TapRoot (relaxation of the 3600 bytes script size standard limit/mempool policy rule), please?
🤔 stratospher reviewed a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118#pullrequestreview-2065078997)
tested ACK f4c588c9.
very cool! i like how the logic depends only on the node we're connecting to.
reproduced the intermittent failure using this patch and verified that the new logic fixes it!
self.nodes[0].add_p2p_connection(P2PInterface(), send_version=False, wait_for_verack=False)
self.connect_nodes(0, 2)
also didn't observe that much of a time increase (only around 70 s) when running all the functional tests in my local config (without wallets).
(https://github.com/bitcoin/bitcoin/pull/30118#pullrequestreview-2065078997)
tested ACK f4c588c9.
very cool! i like how the logic depends only on the node we're connecting to.
reproduced the intermittent failure using this patch and verified that the new logic fixes it!
self.nodes[0].add_p2p_connection(P2PInterface(), send_version=False, wait_for_verack=False)
self.connect_nodes(0, 2)
also didn't observe that much of a time increase (only around 70 s) when running all the functional tests in my local config (without wallets).
💬 stratospher commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1606032360)
wondering if we need the inbound check because an inbound and an outbound connection to the same `TestNode` isn't done. or did you keep it for assurance about the connection direction?
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1606032360)
wondering if we need the inbound check because an inbound and an outbound connection to the same `TestNode` isn't done. or did you keep it for assurance about the connection direction?
💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1606039710)
Ok. The "version handshake" wording refers to the complete initial negotiation round.
(1) outbound send version, (2) inbound send version, (3) inbound send verack, (4) outbound send verack.
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1606039710)
Ok. The "version handshake" wording refers to the complete initial negotiation round.
(1) outbound send version, (2) inbound send version, (3) inbound send verack, (4) outbound send verack.
💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1606038839)
> wondering if we need the inbound check because an inbound and an outbound connection to the same `TestNode` isn't done. or did you keep it for assurance about the connection direction?
Bidirectional connections are allowed and quite common. E.g. in [p2p_disconnect_ban.py](https://github.com/bitcoin/bitcoin/blob/058af75874ffa2b4064e3d6d30cc50f0ec754ba8/test/functional/p2p_disconnect_ban.py#L117) or in [rpc_net.py](https://github.com/bitcoin/bitcoin/blob/058af75874ffa2b4064e3d6d30cc50f0ec754b
...
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1606038839)
> wondering if we need the inbound check because an inbound and an outbound connection to the same `TestNode` isn't done. or did you keep it for assurance about the connection direction?
Bidirectional connections are allowed and quite common. E.g. in [p2p_disconnect_ban.py](https://github.com/bitcoin/bitcoin/blob/058af75874ffa2b4064e3d6d30cc50f0ec754ba8/test/functional/p2p_disconnect_ban.py#L117) or in [rpc_net.py](https://github.com/bitcoin/bitcoin/blob/058af75874ffa2b4064e3d6d30cc50f0ec754b
...
📝 TheCharlatan opened a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141)
The validation caches are currently setup independently from where the rest of the validation code is initialized. This makes their ownership semantics unclear. There is also no clear enforcement on when and in what order they need to be initialized. The caches are always initialized in the `BasicTestingSetup` although a number of tests don't actually need them.
Solve this by moving the caches from global scope into the `ChainstateManager` class. This simplifies the usage of the kernel librar
...
(https://github.com/bitcoin/bitcoin/pull/30141)
The validation caches are currently setup independently from where the rest of the validation code is initialized. This makes their ownership semantics unclear. There is also no clear enforcement on when and in what order they need to be initialized. The caches are always initialized in the `BasicTestingSetup` although a number of tests don't actually need them.
Solve this by moving the caches from global scope into the `ChainstateManager` class. This simplifies the usage of the kernel librar
...
🤔 marcofleon reviewed a pull request: "fuzz: add more coverage for `ScriptPubKeyMan`"
(https://github.com/bitcoin/bitcoin/pull/30134#pullrequestreview-2065088795)
I generated a coverage report for the current scriptpubkeyman harness and for the updated harness in this PR.
Before:
<img width="1175" alt="Screenshot 2024-05-19 at 6 18 27 AM" src="https://github.com/bitcoin/bitcoin/assets/95179662/28d115e6-c217-4c09-b332-914690dba6b0">
<img width="1175" alt="Screenshot 2024-05-19 at 6 18 37 AM" src="https://github.com/bitcoin/bitcoin/assets/95179662/1eff22e5-dfd1-4a52-a222-78c245a633e4">
After:
<img width="1174" alt="Screenshot 2024-05-19 at 7 06 41
...
(https://github.com/bitcoin/bitcoin/pull/30134#pullrequestreview-2065088795)
I generated a coverage report for the current scriptpubkeyman harness and for the updated harness in this PR.
Before:
<img width="1175" alt="Screenshot 2024-05-19 at 6 18 27 AM" src="https://github.com/bitcoin/bitcoin/assets/95179662/28d115e6-c217-4c09-b332-914690dba6b0">
<img width="1175" alt="Screenshot 2024-05-19 at 6 18 37 AM" src="https://github.com/bitcoin/bitcoin/assets/95179662/1eff22e5-dfd1-4a52-a222-78c245a633e4">
After:
<img width="1174" alt="Screenshot 2024-05-19 at 7 06 41
...
💬 laanwj commented on pull request "build: Enable `thread_local` for MinGW-w64 builds":
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2119276179)
> After this and https://github.com/bitcoin/bitcoin/pull/30095, is there any need to keep the thread_local option at all?
+1 i'd also prefer getting rid ot the option.
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2119276179)
> After this and https://github.com/bitcoin/bitcoin/pull/30095, is there any need to keep the thread_local option at all?
+1 i'd also prefer getting rid ot the option.