💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1616438227)
Looks like a slightly non trivial rebase. For context, which PR's triggered it?

💬 Sjors commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1616447514)
A bisect would be ideal. Though you could start with recent pull requests that touch net(_processing).cpp and were not backported to the v25.0 release. E.g. #27626 and #27625.

📝 JayBitron opened a pull request: "exclude ipc scheme from port check "
(https://github.com/bitcoin/bitcoin/pull/28020)
Previous PR https://github.com/bitcoin/bitcoin/pull/22087 cause new error, makes it impossible to use ipc protocol using zmq, this patch will exclude port checking on ipc urls.

💬 nberlin1980 commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1249523602)
This will remove `NET_UNROUTABLE ` and `NET_INTERNAL`, after which they cannot be re-added. Is that intentional (the function name implies it is intentional)? I ask because the other accessors guard against their removal and insertion.

💬 nberlin1980 commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#discussion_r1249529700)
Perhaps add test cases for `RemoveAll` as it relates to these two.

💬 mzumsande commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1616786805)
now the previously unaffected coinstatsindex_tests fails intermittently:
https://github.com/bitcoin/bitcoin/pull/27746/checks?check_run_id=14710590806
(doesn't seem to be related to the PR)
```
72023-07-02T07:40:59.796756Z (mocktime: 2020-08-31T15:34:test/util/index.cpp:15 IndexWaitSynced: Assertion `timeout > SteadyClock::now()' failed.
```

💬 Ayush170-Future commented on pull request "init: adding check for : for -torcontrol flag":
(https://github.com/bitcoin/bitcoin/pull/28018#issuecomment-1616813496)
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.

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