💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578309261)
In commit "kernel: De-globalize fReindex" (9b2c9c2fce32fe858d1361e863c72108a384a5c8)

I think it would be less fragile to assign `blockman_opts.reindex` instead of `chainman.m_blockman.m_reindex`.

Assigning `blockman_opts.reindex` should ensure and make it more obvious that this is only affected by the value of the `-reindex` setting, not the value of a previous setting saved with `WriteReindexing`.

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578320266)
In commit "kernel: De-globalize fReindex" (9b2c9c2fce32fe858d1361e863c72108a384a5c8)

Note: I initially thought there was a minor bug in existing code (independent of this PR) on line 1605 above which checks `if (!options.reindex)` instead of `if (!chainman.m_blockman.m_reindexing)`. It seemed wrong because it could trigger an error suggesting using reindex flag even if reindexing was already happening from a previous request. This behavior is surprising, but on second thought, could actually
...

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578327024)
In commit "kernel: De-globalize fReindex" (9b2c9c2fce32fe858d1361e863c72108a384a5c8)

It seems like there is a prexisting bug here, and the `f_wipe` argument passed here should be `blockman_opts.reindex` instead of `chainman.m_blockman.m_reindexing` or `fReindex`. Otherwise if the node is restarted before reindexing completes the TxIndex will be wiped a second time?

Same for BlockFilterIndex and CoinStatsIndex below

🤔 mzumsande reviewed a pull request: "net: Make AddrFetch connections to fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/26114#pullrequestreview-2020650571)
Thanks for the reviews and sorry for not following up earlier!
I hope I have now addressed all comments.

I also rebased wrt master - strangely github doesn't seem to detect some merge conflicts in this PR, locally I had conflicts with both #28170 and #29850 - different algorithms maybe?

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578349512)
> I agree. I've tested it following `contrib/seeds/README.md` to generate the seeds and the worst scenario was connecting with 5 of the 10.

Yes, I see similar results. If older versions are used, it could become a bit worse though.

> nit: the log might use a variable 2, so that it's defined in a single place.

I have now used a variable for the 2 minutes (sorry, I missed this comment before).

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578356506)
I have reordered the sentence now so that it is clearer now hopefully!

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578357274)
updated the doc!

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578358302)
Good catch! removed the mention of ThreadOpenConnections in the following commit (to keep it out of the already large move commit)

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578355015)
added the 10

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578355641)
done, thanks!

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578354870)
I agree that could be good! Though I think it would add more complexity to this pull, so I would prefer not to do it here - could be a good follow-up though.

Just noting that we do add all fixed seeds to addrman though, so with the connection logic from #27213 we'd eventually still make a connection to the network.

📝 MarnixCroes opened a pull request: "guiconstants: update ORG_DOMAIN to bitcoincore.org"
(https://github.com/bitcoin-core/gui/pull/818)
if desired, can also update `ORG_NAME` to _Bitcoin Core_
<!--
*** 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
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
signific
...

💬 achow101 commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#issuecomment-2075656821)
ACK modulo existing comments.

🤔 ryanofsky reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2020422224)
Updated c45cb13b9e4f6eae50ab6dc42acf471921980591 -> 66022315641934bda301d61f009dae9cb3b45da0 ([`pr/saferesult.2`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.2) -> [`pr/saferesult.3`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/saferesult.2..pr/saferesult.3)) adding change to actually disable the copy constructor

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1578217883)
re: [#29906 (comment)](https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572632242)

> nit: Any reason not to just wrap the args in `std::move`? Seems like the more intuitive approach, and probably slightly more performant?

Actually this turns out not to work because `std::initializer_list` list [does not work](https://old.reddit.com/r/cpp/comments/7a1o7f/why_does_stdinitializer_list_prevents_using/) with move-only types. So unfortunately vector initialization needs to be a little
...

💬 iotamega commented on issue "Manually Banning Peers Results in Crash":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2075676471)
> I just tried in out under ubuntu, and banning both single and multiple peers worked fine. What do you mean with "extensive testing"? If it happens always, just starting the program and banning a peer should be enough? Or does it happen only sometimes for you?

I'd say it happens perhaps 10% of the time, I can ban 100 peers without an issue before it crops up.

I operate more than fifty nodes at this point and pruning them daily gives me some insight to this issue. It isn't a frequent occur
...

💬 Sjors commented on pull request "doc: add LLVM instruction for macOS < 13":
(https://github.com/bitcoin/bitcoin/pull/29934#discussion_r1578401700)
After a full day my virtual box also managed to complete `brew install llvm` without needing a version :-) Will update the PR tomorrow.

💬 laanwj commented on pull request "guiconstants: update ORG_DOMAIN to bitcoincore.org":
(https://github.com/bitcoin-core/gui/pull/818#issuecomment-2075690269)
> if desired, can also update ORG_NAME to Bitcoin Core

The reason that this was never done is because it changes where the Qt settings files are stored. For example this is `~/.config/<ORG_NAME>/<APP_NAME>`.

i'm not sure what actually is the effect of changing the domain, but this shouldn't be done without checking.

💬 hebasto commented on pull request "guiconstants: update ORG_DOMAIN to bitcoincore.org":
(https://github.com/bitcoin-core/gui/pull/818#issuecomment-2075723918)
> if desired, can also update `ORG_NAME` to _Bitcoin Core_

Are there any issues with the current code? If not, then this PR lacks motivation.

📝 kristapsk opened a pull request: "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`"
(https://github.com/bitcoin/bitcoin/pull/29954)
Other node relay settings like `fullrbf` and `minrelaytxfee` are already returned, makes sense to add these two too.