💬 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.
📝 tdb3 opened a pull request: "doc: add guidance for RPC to developer notes"
(https://github.com/bitcoin/bitcoin/pull/30142)
Adds guidance statements to the RPC interface section of the developer notes for data type selection and examples of when to implement `-deprecatedrpc=`.
Wanted to increase awareness of preferred RPC implementation approaches for newer contributors.
I'm sure opinions will differ, so please don't be shy. We want to make RPC as solid/safe as possible.
Example of a discussion where guidelines/context might have added value:
https://github.com/bitcoin/bitcoin/pull/29845#discussion_r157105
...
(https://github.com/bitcoin/bitcoin/pull/30142)
Adds guidance statements to the RPC interface section of the developer notes for data type selection and examples of when to implement `-deprecatedrpc=`.
Wanted to increase awareness of preferred RPC implementation approaches for newer contributors.
I'm sure opinions will differ, so please don't be shy. We want to make RPC as solid/safe as possible.
Example of a discussion where guidelines/context might have added value:
https://github.com/bitcoin/bitcoin/pull/29845#discussion_r157105
...
💬 tdb3 commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#issuecomment-2119308452)
lint error is unrelated. Already discussed in https://github.com/bitcoin/bitcoin/pull/30084#issuecomment-2118676930 and https://github.com/bitcoin/bitcoin/pull/30106.
(https://github.com/bitcoin/bitcoin/pull/30142#issuecomment-2119308452)
lint error is unrelated. Already discussed in https://github.com/bitcoin/bitcoin/pull/30084#issuecomment-2118676930 and https://github.com/bitcoin/bitcoin/pull/30106.
👋 TheCharlatan's pull request is ready for review: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141)
(https://github.com/bitcoin/bitcoin/pull/30141)
💬 jonatack commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1606074422)
The extra whitespace here is the cause of the linter CI failure.
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1606074422)
The extra whitespace here is the cause of the linter CI failure.
💬 jonatack commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#issuecomment-2119312240)
> error could help prevent near term lint issues for open PRs.
@tdb3 Spelling warnings reported by `./test/lint/lint-spelling.py` are printed as warnings but don't cause the lint CI to fail on open PRs and aren't particularly high-priority to fix.
(https://github.com/bitcoin/bitcoin/pull/30084#issuecomment-2119312240)
> error could help prevent near term lint issues for open PRs.
@tdb3 Spelling warnings reported by `./test/lint/lint-spelling.py` are printed as warnings but don't cause the lint CI to fail on open PRs and aren't particularly high-priority to fix.
💬 tdb3 commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1606079769)
Thanks. Fixed
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1606079769)
Thanks. Fixed
💬 pablomartin4btc commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#discussion_r1606087141)
done!
(https://github.com/bitcoin-core/gui/pull/762#discussion_r1606087141)
done!
💬 pablomartin4btc commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#discussion_r1606087218)
done!
(https://github.com/bitcoin-core/gui/pull/762#discussion_r1606087218)
done!
💬 pablomartin4btc commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#issuecomment-2119337707)
Updates:
<ul><li><details>
<summary>Addressed @hebasto's <a href="https://github.com/bitcoin-core/gui/pull/762#pullrequestreview-2058694015">feedback</a>.</summary>
```
git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v
Formatting src/qt/bitcoingui.cpp
Formatting src/qt/utilitydialog.cpp
Formatting src/qt/utilitydialog.h
```
</details></li></ul>
- As part of the above, [functions](https://github.com/bitcoin-core/gui/pull/762#discussion_r1602056777) were re-
...
(https://github.com/bitcoin-core/gui/pull/762#issuecomment-2119337707)
Updates:
<ul><li><details>
<summary>Addressed @hebasto's <a href="https://github.com/bitcoin-core/gui/pull/762#pullrequestreview-2058694015">feedback</a>.</summary>
```
git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v
Formatting src/qt/bitcoingui.cpp
Formatting src/qt/utilitydialog.cpp
Formatting src/qt/utilitydialog.h
```
</details></li></ul>
- As part of the above, [functions](https://github.com/bitcoin-core/gui/pull/762#discussion_r1602056777) were re-
...
💬 brunoerg commented on pull request "fuzz: add more coverage for `ScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/30134#issuecomment-2119342047)
> I generated a coverage report for the current ScriptPubKeyMan harness and for the updated harness in this PR.
@marcofleon It's good to mention how you run it. How many hours? From seed corpus (note that changes can invalidate it)?
(https://github.com/bitcoin/bitcoin/pull/30134#issuecomment-2119342047)
> I generated a coverage report for the current ScriptPubKeyMan harness and for the updated harness in this PR.
@marcofleon It's good to mention how you run it. How many hours? From seed corpus (note that changes can invalidate it)?
💬 mjdietzx commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#issuecomment-2119495691)
I addressed @achow101 's comments:
- re [this comment](https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567805211), I removed `random.sample`. it's unnecessary
- re [this comment](https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567795960), I removed the unnecessary explanation of where descriptor came from
Thank you all for your reviews and patience
(https://github.com/bitcoin/bitcoin/pull/29154#issuecomment-2119495691)
I addressed @achow101 's comments:
- re [this comment](https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567805211), I removed `random.sample`. it's unnecessary
- re [this comment](https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567795960), I removed the unnecessary explanation of where descriptor came from
Thank you all for your reviews and patience
💬 mjdietzx commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1606146880)
I just pushed the changes now 🤦♂️
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1606146880)
I just pushed the changes now 🤦♂️