💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1287493545)
Ugh, I was inclined to argue with you because I think the tests should be as tight as possible. But realistically, it's more likely for a class to be forgotten in the checks (as has happened here) than a false-positive in some future class. So I begrudgingly agree.

💬 ALIR86 commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1670094367)
I install in a we3 new wallet ,I have 2 unit 2 BTC in my profile I don't underestand your means that you haven't authorized to merge ,merge in a blockchain else just there unfortunately someone with now of forks of staking withdrawals my coins zBzTC legally me swhouse and get profit of staking of forks it hummm?! Truly interest

💬 ALIR86 commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1670100363)
Why to offer you why I donot are authorized to merge this pull request tell me with now who use of my forks and staking my profile 2 unit BTC me?@'📈🤔🥵☹️ @

🤔 furszy reviewed a pull request: "fuzz: improve `coinselection`"
(https://github.com/bitcoin/bitcoin/pull/27585#pullrequestreview-1566336739)
Code review ACK 1e0f5f088.

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1287502527)
what about asserting inputs are actually added and checking the total weight?

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1286505964)
Could also check the resulting inputs size and the `use_effective_fee` field.

Also, we only use of the `Merge` function in the sources to combine a `MANUAL` selection with a one of the automatic selections. We never combine two srd solutions.

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1287503891)
what was the reason behind this change?

🤔 pinheadmz reviewed a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1567795941)
code review ACK at d54e7dad7513ab4c587456b56d3a838daa151e4d

built and passed all tests

However as noted below I think the approach can be simplified in regards to local services.

💬 pinheadmz commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1287433864)
I don't think you need to use `sprintf` here? Might be left over from the `incoming only` thing you just updated?

💬 pinheadmz commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1287479200)
I might be misunderstanding something here, but I think this chunk can be moved to `net_processing.cpp` `InitializeNode()` because:
- the `CNode` will already have its `m_permission_flags` set in `ConnectNode()`
- `m_permission_flags` is all that's needed to set `our_services` inside `InitializeNode()`
- then you won't need to use `std::optional<std::pair<CNode*, ServiceFlags>> ` which is, hey, a real banger of a type!

Otherwise I don't see a reason to compute and then pass around those se
...

💬 hebasto commented on pull request "Release `LockData::dd_mutex` before calling `*_detected` functions":
(https://github.com/bitcoin/bitcoin/pull/26781#issuecomment-1670156374)
cc @ryanofsky

💬 TheCharlatan commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#issuecomment-1670237772)
Concept ACK

💬 ryanofsky commented on pull request "refactor: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager":
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1287637273)
re: https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1286059790

> nit: Add missing `{}` on touched line for multiline if?

Sure, done

💬 ryanofsky commented on pull request "refactor: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager":
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1287637413)
re: https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1286059126

> nit: Missing clang-format on touched line?

Thanks, ran clang-format-diff

🤔 ryanofsky reviewed a pull request: "refactor: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager"
(https://github.com/bitcoin/bitcoin/pull/28218#pullrequestreview-1568100631)
Updated 3d70aa3c60f07ed6c9eed237b2c82a59674239cf -> bda557e5ebe7f754984a34ddfd33d2540c0303b9 ([`pr/assumeibd.2`](https://github.com/ryanofsky/bitcoin/commits/pr/assumeibd.2) -> [`pr/assumeibd.3`](https://github.com/ryanofsky/bitcoin/commits/pr/assumeibd.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/assumeibd.2..pr/assumeibd.3)) with suggested changes

💬 ryanofsky commented on pull request "refactor: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager":
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1287637346)
re: https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1286059909

> Same?

Done

💬 instagibbs commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#issuecomment-1670290088)
Looks like this lost `TrimToSize` changes?

```
commit ed6045a8f99f14987977e13faa1865c3bf56890f
Author: glozow <gloriajzhao@gmail.com>
Date: Tue Jan 17 13:43:27 2023 +0000

[mempool] evict everything below min relay fee in TrimToSize()

At this point it's not expected that there are any such transactions,
except from reorgs and possibly when loading from mempool.dat.
```

I rely on this for ephemeral anchors, trying to rebase.

💬 instagibbs commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1287681547)
I don't think this case is actually covered

rebased branch is failing on this test: https://github.com/bitcoin/bitcoin/pull/26403/files#diff-d18bbdec91d0f4825b512a31f34666b000c7b7e2e05a3d43570c4b971532616fR409

💬 mzumsande commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1670349722)
> I can't think of a scenario where you're sure that you can remove from mapBlockSource unless the peer disconnects.

I agree. If we could immediately remove from `mapBlockSource` in `ProcessBlock` in all cases, there wouldn't really be a need to have a map for this info in the first place. It's purpose, I think, is to store the information who provided us with that not fully validated block, so the peer is still available for potential punishment in case we get to fully validate it at a later
...

🤔 furszy reviewed a pull request: "doc: Add example of how to mix private and public keys in descriptors"
(https://github.com/bitcoin/bitcoin/pull/27414#pullrequestreview-1568518583)
Concept ACK

+1 for squashing the commits.