💬 luke-jr commented on pull request "Prevent file descriptor exhaustion from too many RPC calls":
(https://github.com/bitcoin/bitcoin/pull/27731#discussion_r1249842846)
I'm thinking of the case where you build with 2.2.1, then try to run it with 2.1.12. (If that's not supported, the runtime check is unnecessary)

💬 luke-jr commented on pull request "init: deduplicate added connections":
(https://github.com/bitcoin/bitcoin/pull/27804#issuecomment-1617016570)
Weak approach NACK. This feels like a hacky workaround for what is ultimately a race condition. Rather we just address the cause of the issue, instead of introducing weird variables (eg, what happens if the `-connect`ion is lost, and the user use the `addnode` RPC right at the correct moment?).

💬 luke-jr commented on pull request "Blocking arguments -nohelp, -noh, and -no?":
(https://github.com/bitcoin/bitcoin/pull/27814#issuecomment-1617019869)
nit: Commit summary is too long. Would also be nice to rebase on top of 8afa602f308ef003bb6893718eae1fe5a830690c

💬 luke-jr commented on pull request "CLI: Only one Request Handler can be specified.":
(https://github.com/bitcoin/bitcoin/pull/27815#issuecomment-1617024007)
nit: Commit summary is too long. Would also be nice to rebase on top of 42af9596ce85a541988abee54eed8a9b271a46a1

🤔 luke-jr requested changes to a pull request: "Supporting parameter "h" and "?" in -netinfo."
(https://github.com/bitcoin/bitcoin/pull/27830#pullrequestreview-1510054476)
Concept NACK. This seems to be for `bitcoin-cli -netinfo h`, which is weird and there's no reason to expect that to work.

💬 kevkevinpal commented on pull request "init: adding check for : for -torcontrol flag":
(https://github.com/bitcoin/bitcoin/pull/28018#issuecomment-1617123715)
> Overall, I think the code is good. However, there have been some earlier remarks in this [comment](https://github.com/bitcoin/bitcoin/pull/23605#issuecomment-980045955) on where to place this code.
>
> Someone with a better understanding of the entire codebase should comment on the placement of this.
>
> In any case, I'm testing it locally and will report back on whether it meets the requirements soon (one suggestion I think you should avoid using `/` in the branch name)

Yea I could t
...

👍 TheCharlatan approved a pull request: "test: move remaining rand code from util/setup_common to util/random"
(https://github.com/bitcoin/bitcoin/pull/27425#pullrequestreview-1510492158)
ACK 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a

💬 S3RK commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#discussion_r1250387177)
Isn't the fee for all outputs accounted for in `not_input_fees`? It seems to me this will always overpay and then will be corrected when we detect surplus fee.

Maybe a clearer way to handle this would be to pass `chnage_fee=0` to `GetChange` if `existent_change_out_index=true`. I'd prefer to minimise number of cases when we need to correct fees at the later stage of transaction building.

💬 S3RK commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1617526433)
How would it work if coinselection produced result without change output?

💬 MarcoFalke commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1617532581)
Yeah, should be caused by an upstream CI change. I guess there should be no risk in changing 120s to 300s, even potentially fixing the issue you see.

🤔 MarcoFalke reviewed a pull request: "test: move remaining rand code from util/setup_common to util/random"
(https://github.com/bitcoin/bitcoin/pull/27425#pullrequestreview-1510582779)
lgtm ACK 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a 🐤

<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: lgtm ACK 5b3f6a49e6bda9f5
...

💬 MarcoFalke commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1250420188)
in 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a:

Some notes:

* I wonder if this should still be called "insecure", as the underlying randomness is no longer insecure, just deterministic, which is obvious in a test env. Maybe call it MockedRandBytes?
* `static` isn't needed?
* I wonder if it makes sense to wrap all member functions to global functions. Why not just call the member function of the global? (Before maybe renaming it to `g_mocked_random` or `g_test_random` or so? This would allow
...

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1246207242)
in "Bump unconfirmed parent txs to target feerate" 8e3d924f02ee28043e8844a301389915823e5893

I think all the tests would be easier to understand if we start with an empty testing wallet and send unconfirmed txs to it from `def_wallet`.

Now we start with the testing wallet with already one confirmed input, but why if we test spending mostly unconfirmed txs? Current setup requires reader to do more coin tracking in mind.

🤔 S3RK reviewed a pull request: "Bump unconfirmed ancestor transactions to target feerate"
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1504669255)
Started reviewing tests, but need more time.

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1246215741)
do we want to check naïve ancestry fee rate or the fee_rate at which it will be accepted at a block (i.e. ancestry score)?

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1246199879)
in "Bump unconfirmed parent txs to target feerate" 8e3d924f02ee28043e8844a301389915823e5893

nit: better use a multiple of `target_fee_rate`

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1250425745)
that seems like a duplicate check of `assert_undershoots_target`

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1250394244)
Oops, sorry about misleading you 🙄

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1250419037)
My idea was to add `bump_fee_group_discount` within `GetTotalBumpFee()` function. Probably I'm missing something but I don't know what. Why wouldn't it just work?

📝 fanquake opened a pull request: "docs: fixup honggfuzz fuzz patch"
(https://github.com/bitcoin/bitcoin/pull/28021)
Closes #28019.