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

💬 MarcoFalke commented on pull request "doc: Add example of how to mix private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/27414#issuecomment-1670689584)
I think this can be closed "Up for grabs"? Does someone want to pick it up?

💬 MarcoFalke commented on pull request "doc: Add example of how to mix private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/27414#issuecomment-1670691549)
For reference, the docs on how to squash commits: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 ajtowns commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1670725416)
There's no difference between "reserving" an opcode and "disabling" it other than the error message that's passed through when it's used. Having different error messages for "error even if not executed" and "error only when it's executed" might make sense, but that isn't what we currently do (OP_VERIF/OP_VERNOTIF are exceptions) and isn't what this PR currently does.

Having the error codes be:

```
case SCRIPT_ERR_BAD_OPCODE:
return "Attempted to execute invalid/reserv
...

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288005314)
One is a `std::vector`, the other is `UniValue`, no?

💬 ajtowns commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1670764585)
> All of those transactions are taken from my OpenTimestamps calendars. My OTS calendars bump fees repeatedly, increasing the feerate by 1sat/vb with each replacement, starting at approximately the minimum relay fee.

Yes, that is a pretty perfect mechanism if you want to create a false positive for "full rbf" when all you're actually triggering is eviction from the mempool due to a change in minfee. For nodes that don't run "full rbf", they'll see one of the early transactions, add it to the
...