💬 sr-gi commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1493027486)
This was added by me as a suggestion from @vasild in https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296832854 to maintain the older logic, but it comes all the way from https://github.com/bitcoin/bitcoin/commit/6387f397b323b0fb4ca303fe418550f5465147c6#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02R430.

There is no comment nor commit message motivating it though.

I think randomizing the order in which the resolved addresses are tried wouldn't hurt though.

💬 sr-gi commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1493029119)
I'm a bit curious about this. I think I'll try to dig into how bad could it be if a malicious DNS seed provides us with 256+ addresses that try to delay the TPC handshake as much as possible

💬 sr-gi commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#issuecomment-1949408988)
Thanks for the reviews @mzumsande @vasild and @naumenkogs!

I have some updates pending on the nits provided by @naumenkogs, but I'll wait until we resolve https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1492192396 to push them all. I'd also look into https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1491629378 out of curiosity

💬 cbergqvist commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1949439983)
Reviewed 58cb22c.

Great to finally add support for JSON-RPC 2.0, thanks for doing this!

(Super-late concept reflection: it might be more modern to provide something like [OpenAPI](https://swagger.io/) instead of JSON-RPC in the future ([HN discussion](https://news.ycombinator.com/item?id=34211796)). This would facilitate using tools like Postman and more easily signal which requests are cacheable/idempotent etc. On the other hand the codebase already has Cap'n'Proto and ZMQ, so also makes
...

💬 aureleoules commented on pull request "test: add missing tests for Assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29428#issuecomment-1949561588)
> Looks like [corecheck.dev/bitcoin/bitcoin/pulls/29428](https://corecheck.dev/bitcoin/bitcoin/pulls/29428) is dead?

@maflcko thanks for the ping. The code I added to filter out flaky coverage was too slow and isn't very reliable so I removed it. Unfortunately, the UI will now display flaky coverage.

💬 1440000bytes commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1949572923)
Are ocean contributors (miners) complying with the nodes?

💬 ariard commented on pull request "Add imbued v3 based on template-matching":
(https://github.com/bitcoin/bitcoin/pull/29427#issuecomment-1949579407)
one design feedback, look on how any imbuance mechanism would have to fit with dynamic upgrades, whatever the bitcoin use-cases. especially matters for time-sensitive flows: https://github.com/lightning/bolts/pull/1117

💬 mrsteve0924 commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-1949611717)
after 4 years why not close this? obvious no one is going to fix it

💬 ismaelsadeeq 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#discussion_r1493302062)
Maybe weshould also check that we can have more than two directly conflicting transactions as long as they are all in a cluster size <=2.

<details>
<summary>diff </summary>

```diff
diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp
index e15fd29da3..561ebd3881 100644
--- a/src/test/rbf_tests.cpp
+++ b/src/test/rbf_tests.cpp
@@ -431,6 +431,53 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup)
BOOST_CHECK(new_diagram[1] == FeeFrac(high_fee, low_size
...

🤔 ismaelsadeeq reviewed a pull request: "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2"
(https://github.com/bitcoin/bitcoin/pull/29242#pullrequestreview-1886534107)
Thanks for adding the test!
re-ACK d57fbda350ee9051931d9a0dad4beb55f6b2e574

💬 ismaelsadeeq 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#discussion_r1493302276)
here an another place below!

💬 maflcko commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1949937829)
Yeah, good point. So I guess the only way to remove unused includes would be to write another check in the linter. I'll drop the last commit and leave everything else for a follow-up.

💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1949962146)
re-ACK d5228efb5391b31a9a0673019e43e7fa2cd4ac07 🌄

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK d5228efb5391b31a9a06
...

🤔 hebasto reviewed a pull request: "Prevent weird focus rect on inital sync"
(https://github.com/bitcoin-core/gui/pull/795#pullrequestreview-1886576526)
I can actually see two distinct issues here:

1. The "Hide" button doesn't have focus when the overlay shows for the first time. This can be resolved with `ui->closeButton->setFocus();` in the `ModalOverlay` constructor.

2. The other issue is a bit convoluted. As a child widget, the overlay also follows its parent's tab order when processing "Tab" or "Shift+Tab" keys. Isolating the overlay's tab order might also be a solution to the issue.

This PR indeed forces focus on the "Hide" as lon
...

💬 hebasto commented on pull request "Prevent weird focus rect on inital sync":
(https://github.com/bitcoin-core/gui/pull/795#discussion_r1493340467)
Using the same variable across all conditions for the same action improves readability:

```suggestion
if (layerIsVisible) {
```

💬 jadijadi commented on pull request "Keep focus on "Hide" while ModalOverlay is visible":
(https://github.com/bitcoin-core/gui/pull/795#issuecomment-1950222937)
> I can actually see two distinct issues here:
>
> 1. The "Hide" button doesn't have focus when the overlay shows for the first time. This can be resolved with `ui->closeButton->setFocus();` in the `ModalOverlay` constructor.
> 2. The other issue is a bit convoluted. As a child widget, the overlay also follows its parent's tab order when processing "Tab" or "Shift+Tab" keys. Isolating the overlay's tab order might also be a solution to the issue.
>
> This PR indeed forces focus on the "Hi
...

💬 hebasto commented on pull request "Keep focus on "Hide" while ModalOverlay is visible":
(https://github.com/bitcoin-core/gui/pull/795#issuecomment-1950228993)
cc @jarolrod @furszy

💬 sdaftuar 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#discussion_r1493358857)
I think we should drop the `Assume()` calls, as otherwise I think we ought to do more to ensure that using operator- on FeeFrac's is safe everywhere that might be invoked.

💬 daniel-btc-nym commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1950254374)
> I like bugfixes the most, when they are touching only a single line of code. But if more changes are required, then that is fine, too.

My changes have a few orthogonal, but related fixes and test additions. We could make different pull-requests for them. Let me know what you prefer.

For example:
1. Check strncmp will not go out of bound. This is lines 99-115 in univalue_read.cpp ; unit tests that call `no_reading_beyond_end_test`
1. Change parsing of numbers, where there were two plac
...

🤔 tdb3 reviewed a pull request: "Update rpcauth.py"
(https://github.com/bitcoin/bitcoin/pull/29433#pullrequestreview-1886951173)
Concept ACK. Inline comments provided.