💬 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.

💬 tdb3 commented on pull request "Update rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/29433#discussion_r1493397894)
The `share/rpcauth/README.md` file should also be updated to explain the argument and capability change in `rpcauth.py`.

nit:
Recommend making the argument be a single letter (e.g. `-j` for json) to follow the existing style with `-h` for help and prevent conflation with `-j -s -o -n`.

💬 bstin commented on pull request "Update rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/29433#discussion_r1493398180)
thank you. yes single "-" makes more sense.

💬 fjahr commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1950323631)
> Being able to specify a start_height sounds like something that would be generally useful for all indexes, not just this one. I've been toying with the idea of what it would take to have a "pruned" txindex and I would need to be able to start from a specific height for that. @fjahr I took the branch you started and got it working with a little tinkering here: https://github.com/josibake/bitcoin/tree/add-start-height-to-base-index . With a little more work this could probably be its own PR. Fee
...

🤔 sdaftuar 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-1886598977)
Still reviewing all the tests -- leaving some nits that I've noticed so far.

In addition:
* I commented [here](https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1493358857) that I think we should drop the `Assume(size != 0 || fee == 0)` calls in feefrac.h, so that we don't have to police all the places we invoke operator- to make sure we can never accidentally violate that.
* Another nit: in the commit message for commit 860823fb93212fa78ab928589bd403da462be222, it should say "Fee
...

💬 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_r1493359169)
nit: As the functions here don't relate to the `FeeFrac` implementation, perhaps this should be in a different file? `feerate_diagram.cpp` maybe?

💬 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_r1493390467)
nit: Could add a comment here more precisely defining the preconditions for this function and what definition we are using for feerate diagram: specifically that a feerate diagram consists of a list of (fee, size) points with the property that size is strictly increasing and that the first entry is (0, 0). (If these conditions are violated then the function can fail with an `Assume()` failure.)

💬 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_r1493359700)
nit: "direction coefficient" is not a term I'm familiar with (and from googling it doesn't seem like a super common term); perhaps it would be clearer to just use the word "slope" here?

💬 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_r1493401259)
nit: Maybe update the comment to be more explicit that this function only works for the situation where (replacement_fees, replacement_size) corresponds to a transaction package that has no in-mempool dependences.

💬 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_r1493391184)
nit: "fee vs. size diagram" -- maybe we should standardize the terminology to "feerate diagram"? I was also thinking about adding some documentation to doc/policy/mempool-replacements.md explaining what we're doing now.

💬 fjahr commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1493405713)
Why hardcode this value here when we can get it from chain params?

💬 sipa 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_r1493417950)
Oh, I seem to have translated literally from Dutch. "Slope" is indeed the proper English translation.

💬 fjahr commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1493429857)
Removing this is causing an error on all non-mainnet chains now because m_start_height is 0 by default, which is the genesis block and it needs to be skipped because it's unspendable and has no undo data.