📝 kevkevinpal opened a pull request: "init: adding check for : for -torcontrol flag"
(https://github.com/bitcoin/bitcoin/pull/28018)
right now for -torcontrol you can pass in any string as long as it doesn't have a : but that is invalid. I added an additional check before calling SplitHostPort to see if there is a :


closes https://github.com/bitcoin/bitcoin/issues/23589
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
firs
...

💬 kevkevinpal commented on pull request "init: adding check for : for -torcontrol flag":
(https://github.com/bitcoin/bitcoin/pull/28018#issuecomment-1615220421)
<img width="500" alt="Screenshot 2023-06-30 at 4 40 24 PM" src="https://github.com/bitcoin/bitcoin/assets/15950706/d95ba8f9-dad2-4ada-b235-41acf8b58aa4">

-----

<img width="500" alt="Screenshot 2023-06-30 at 4 40 12 PM" src="https://github.com/bitcoin/bitcoin/assets/15950706/d2280a41-b1c2-44e6-b32a-9112321b1664">

-----

<img width="500" alt="Screenshot 2023-06-30 at 4 39 51 PM" src="https://github.com/bitcoin/bitcoin/assets/15950706/ec94182a-b9dd-4701-b57d-6a45ad3487b3">

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248314125)
> I see, so maybe current code is buggy and returns bad rpc results then?

yeah.

> I guess simplest thing to do is just document GetFirstStoredBlock's odd behavior in this case and keep it unchanged, rather than returning nullptr which could segfault, or adding an assertion.

Have struggled a bit with this.
I tried to maintain the previous `GetFirstStoredBlock` behavior but it conflicts so much with the function's description: "Find the first stored ancestor of 'start_block' immediately
...

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248333228)
hm, this one was intentional. I edited `AddTestNode` and it was previously missing a whitespace between functions

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248337602)
removing the optional from the out param means that the compiler can't guarantee that the value of `network` was set. so `preferred_net` needs a default value, which I set to `NET_UNROUTABLE` and then checked for later in the function. this approach doesn't feel much cleaner to me than the previous of having network be optional, but I don't feel strongly either way. or, is there a better solution I'm not seeing right now?

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248337827)
changed back to a map, so this is relevant again. removed the explicit initialization due to the default behavior you mentioned. thanks!

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248337990)
fixed

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248338620)
thanks! okay, fixed and confirmed that none of the current patch invokes `ConnectedThroughNetwork` anymore :)

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248338719)
ah, good catch. updated

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248338974)
updated and double checked that clang-format doesn't have any other suggestions for the patch

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248339085)
ah I see. okay updated :)

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1615277942)
latest push returned to map & addressed review comments

@vasild
I very much agree with your line of questioning. storing as a data structure definitely has some explicit tradeoffs to counting the number on the fly. neither is a clear winner for me, so I'm fine implementing whatever reviewers prefer.

> It is also not generic - the "full outbound or manual" number is used only in this specific case and unlikely to be used elsewhere in the future. A more generic approach, that could possib
...

👍 luke-jr approved a pull request: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411#pullrequestreview-1508023222)
utACK e7cf8657e1165ea5da3911a9e543837cd8938f97

Approach: Unsure about the refactoring, but ok

💬 luke-jr commented on pull request "Switch RPCConsole wallet selection to the one most recently opened/restored/created":
(https://github.com/bitcoin-core/gui/pull/696#discussion_r1248375619)
It's the pointer that's `const` here. Seems pretty reasonable, though it only has a net effect of preventing accidentally changing `wallet_model` in this method.

💬 luke-jr commented on pull request "Switch RPCConsole wallet selection to the one most recently opened/restored/created":
(https://github.com/bitcoin-core/gui/pull/696#discussion_r1248375640)
The `const` has no effect here.

💬 tuanggo commented on issue "ThreadI2PAcceptIncoming temporarily bypasses 125 connection ceiling":
(https://github.com/bitcoin/bitcoin/issues/27843#issuecomment-1615739130)
ok

💬 ajtowns commented on pull request "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings":
(https://github.com/bitcoin/bitcoin/pull/27401#issuecomment-1615756925)
utACK

Manually setting that flag by overriding CXXFLAGS disables a whole bunch of debugging warnings, so handling it directly seems very helpful.

💬 pablomartin4btc commented on issue "Unable to open descriptor wallets that are not in a wallet directory":
(https://github.com/bitcoin/bitcoin/issues/26739#issuecomment-1615886802)
> I'm not sure that this is something that we should implement. This issue is primarily for brainstorming and comment gathering.

I understand, and just for my own sake, I'd need to play around with the code and see how it behaves respect to the issues described above in @ryanofsky's comment, plus the "downgrading issues" discussion between both of you, and also the conversations around situations detailed on both PRs #1889 and #11687.

💬 darosior commented on pull request "Miniscript: always treat unsatisfiable scripts as insane":
(https://github.com/bitcoin/bitcoin/pull/27997#issuecomment-1615891160)
I know, my point is the definition as spelled out before would still hold (anybody would be able to spend it but the "apparent policy of this miniscript match its actual semantics"). What i was trying to get at is that the rationale for requiring sane miniscripts to contain a signature check isn't to make the actual semantics match the apparent policy, but that it's unusable otherwise. And it's the same for unsatisfiable miniscripts.

Anyways, i was just nitpicking. I pushed an update to leave
...

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1615894814)
I think the `CHECK_NONFATAL` that we don't parse Miniscript descriptors with an empty set of keys can be removed here, No need to hold off this PR and it was added in https://github.com/bitcoin/bitcoin/pull/27997 which fixes the bug it uncovered.