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

💬 ajtowns commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1670499129)
> I have not lost interest I simply do not have the technical expertise to maintain this

Closing and marking as up for grabs.

✅ ajtowns closed a pull request: "Remove -mempoolfullrbf option"
(https://github.com/bitcoin/bitcoin/pull/26525)

💬 ajtowns commented on pull request "net_processing: re-allow fetching of genesis block via `getblockfrompeer`":
(https://github.com/bitcoin/bitcoin/pull/28205#issuecomment-1670573469)
Concept NACK, I think -- why use the network to retrieve something that's already hardcoded? Easy to special case this in `getblock` I think:

```c++
static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex* pblockindex)
{
CBlock block;
{
LOCK(cs_main);
if (blockman.IsBlockPruned(pblockindex)) {
if (pblockindex->nHeight == 0) {
return blockman.GetParams().GenesisBlock();
}
throw JSONRPCError(RP
...

💬 ajtowns commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1670579639)
I don't understand why we'd want to touch consensus code for something that's at most a slightly confusing error message? Just changing the error description as in #28234 seems a strictly better approach.

As it stands, `SCRIPT_ERR_DISABLED_OPCODE` covers opcodes that are unusable even in unexecuted IF/ELSE branches, while `SCRIPT_ERR_BAD_OPCODE` covers opcodes that are only unusable when evaluated.

💬 jarolrod commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1287879314)
I'm catching up on the syntax for Github Actions, but wouldn't this run this CI task twice when a Pull request is opened? That seems to be the case when opening this against my fork: https://github.com/jarolrod/bitcoin/pull/2

💬 ChrisCho-H commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1670599690)
@ajtowns https://github.com/bitcoin/bitcoin/pull/28234 is for reserved opcode, but this change is for disabled opcode.
Moreover, it's not just about confusing err msg but also for the efficiency which I said as a second reason above.
I agree that we need to be conservative for consensus code, though I don't see the side effect of this change(let me know if there is!).

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1287941903)
> Building on your new approach, the non-default parameters could potentially be handled with pointers, to avoid unnecessary copy operations.

I don't think your approach works. It is not possible to "move" a const reference into a shared_ptr. It will be a copy again.

Simply using a raw pointer in your approach seems fine, though.

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1287948475)
Mind creating a pull with the outstanding feedback? :)

If not, I'll try to do it next week.