🤔 furszy reviewed a pull request: "test: Remove redundant verack check"
(https://github.com/bitcoin/bitcoin/pull/30252#pullrequestreview-2106373554)
Code ACK fa6627706580ea with a comment.

💬 furszy commented on pull request "test: Remove redundant verack check":
(https://github.com/bitcoin/bitcoin/pull/30252#discussion_r1632399010)
It would be nice to be explicit about the race conditions rather than keeping this vague. Maybe could add something like: "The node will ignore all messages received during the version handshake window".

💬 stickies-v commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1632419791)
I suggested the current approach, see https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1631387475

> I don't think this is used anywhere.

What do you mean?

🤔 theStack reviewed a pull request: "util: add BitSet"
(https://github.com/bitcoin/bitcoin/pull/30160#pullrequestreview-2106417903)
Reviewed 1e08ba37e7b5d9b083c938d90526f8fabbe1c799 so far, the code looks correct to me. Planning to review and run the fuzz test tomorrow.

💬 theStack commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1632438115)
nit: same as done in `operator++`, could add `Assume(m_val != 0)` here as well (both for `IntBitSet` and `MultiIntBitSet` iterators)

🤔 tdb3 reviewed a pull request: "cli: restrict multiple exclusive argument usage in bitcoin-cli"
(https://github.com/bitcoin/bitcoin/pull/30148#pullrequestreview-2106509791)
Looks like commits 628d2d4173660c1ad35b2d84524f71b92593d7cd and ac0d2e474b5df93c8e5824d4d326eda977aade3d overlap a bit. For example:

https://github.com/bitcoin/bitcoin/blob/628d2d4173660c1ad35b2d84524f71b92593d7cd/src/bitcoin-cli.cpp#L1178-L1182

How about squashing some of these changes to help keep the master branch commit log cleaner?

💬 furszy commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1632512298)
> I suggested the current approach, see https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1631387475

Hmm ok. We should probably go further and deduplicate the init.cpp / setup_commons.cpp code somewhere in the future.

> > I don't think this is used anywhere.
>
> What do you mean?

This is part of the unit test framework and no unit test, benchmark or fuzz test seems to make use of it. "-reindex" and "-reindex-chainstate" are always unset.

📝 threewebcode opened a pull request: "chore: fix typos"
(https://github.com/bitcoin/bitcoin/pull/30259)
fix the typos in the code comments

<!--
*** 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
significantly:

* Any test
...

💬 maflcko commented on pull request "test: Remove redundant verack check":
(https://github.com/bitcoin/bitcoin/pull/30252#discussion_r1632619229)
I think the name `fSuccessfullyConnected==false` implies that the connection may not be successfully used (yet), so I think I'll leave it as-is for now.

Also, I think "Will ignore *all* messges received" isn't true, because the node will accept some message types.

What about: "Some message types are blocked from being sent or received before `fSuccessfullyConnected`."?

💬 S3RK commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#issuecomment-2157495123)
light code review ACK 24bc46c83b39149f4845a575a82337eb46d91bdb

💬 S3RK commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1632757408)
nit: it makes sense to keep this check before coin selection to return early and avoid wasting time

💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1632790468)
Yeah, I had considered this but it would require refactoring `IsDust` in a non-trivial way, since `IsDust` is used outside of the wallet and as such doesn't really know what a `CRecipient` is.

It's definitely possible, since all you really need to check for dust is the serialized size of the output and the value, but doing the refactor here blows up the scope of this PR quite a bit. I think it would be better to handle this in a separate PR, i.e. refactor `IsDust` so it can be used for both `
...

👍 hebasto approved a pull request: "build: Remove --enable-gprof"
(https://github.com/bitcoin/bitcoin/pull/30257#pullrequestreview-2106998558)
ACK fa780e1c25e8e98253e32d93db65f78a0092433f, I have reviewed the code and it looks OK.

🚀 fanquake merged a pull request: "refactor: performance-for-range-copy in psbt.h"
(https://github.com/bitcoin/bitcoin/pull/30253)

💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1632823304)
Actually, I just realized I was over complicating this: you don't need to refactor `IsDust`, you just need to wrap it with an `IsDust` function that turns a `CRecipient` into a `CTxOut`, same as `GetSerializeSizeForRecipient`. Will update.

👍 fanquake approved a pull request: "build: warn on self-assignment"
(https://github.com/bitcoin/bitcoin/pull/30235#pullrequestreview-2107065005)
ACK 15796d4b61342f75548b20a18c670ed21d102ba8 - not a huge fan of inline pragma usage, but this seems fine, given it's to work around an already-fixed compiler bug, and we'll only be carrying it for a shortish time in any case.

🚀 fanquake merged a pull request: "build: warn on self-assignment"
(https://github.com/bitcoin/bitcoin/pull/30235)

💬 fanquake commented on pull request "Enable clang-tidy checks for self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2157716676)
> @fanquake Would you be ok with carrying this in our subtree? I'm not sure of a better solution.

Yea I think that's fine.

💬 fanquake commented on pull request "guix: use GCC 13 to builds releases":
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1632843604)
Wondering if it's just sporadic, given previous pushes passed, i.e https://cirrus-ci.com/task/5429980706897920, and it's doesn't look like much has changed upstream recently. Will push again soonish.

💬 glozow commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-2157725577)
ACK 429ec1aaaa

Thank you for rebasing!

💬 maflcko commented on pull request "guix: use GCC 13 to builds releases":
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1632851441)
Yeah, I guess it is similar to the intermittent wine errors on master that are already happening too frequent