💬 maflcko commented on pull request "test: Fix utxo set hash serialisation signedness":
(https://github.com/bitcoin/bitcoin/pull/29399#discussion_r1484406087)
Did that in https://github.com/bitcoin/bitcoin/pull/29401

💬 instagibbs commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#issuecomment-1936047945)
concept ACK

might be good to recap why it was only added in BLOCK processing but not other `ProcessBlocks`: In every other case we already don't punish the sender of compact blocks for failing these higher level checks, while full blocks allow for punishment.

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484425597)
re: https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484322490

> In any case `m_substream{substream}` still creates a copy when it shouldn't, due to a missing `std::forward`, no?

Oh that's interesting. I wouldn't think to use `std::forward` because normally it's just used for && rvalue reference function template parameters, not class template parameters, and doesn't do anything useful if && is not used and special template deduction rules for it are not applied.

We are actual
...

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484427635)
nit: The reference to "in-package" doesn't fit here.

💬 epiccurious commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1936072870)
Concept ACK 7dc0442ff5013827c051c8ff50c8fd8aa9440d7c.

💬 epiccurious commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#issuecomment-1936081626)
re-ACK 5b358cdd1a5f5d2fe87a9e41c638996eab2e2796.

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484436443)
> I could imagine this being useful if you wanted to write something like `ParamsStream mystream{FileStream{"file.bin"}, param1, param2}` and have the file automatically closed when the stream was destroyed.

Yes, an example would be in net.cpp, but feel free to ignore, if it is too much hassle.

```cpp
src/net.cpp- DataStream underlying_stream{vSeedsIn};
src/net.cpp: ParamsStream s{underlying_stream, CAddress::V2_NETWORK};

💬 epiccurious commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1936096250)
Concept ACK 0eec878e514c23d33d2cd81c44566ea601eb263f.

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484467280)
I think if I comment out these lines of code, no unit tests or functional tests fail -- is it possible this isn't covered anywhere? (I didn't run the fuzzer with this change)

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1936174128)
ACK 29029df5c700e6940c712028303761d91ae15847

In addition to my code review and running a slightly earlier version of this branch on a year's worth of historical data, I also did a bunch of mutation testing to confirm code coverage for every component of the new logic (found one block that I think is unreachable, which I commented on above, but doesn't seem like an issue either way).

⚠️ derekm opened an issue: "Enable sanctions enforcement in Spam Filter Code"
(https://github.com/bitcoin/bitcoin/issues/29416)
### Please describe the feature you'd like to see added.

As a miner operating within a sanctions regime,
I need to be able to reject transactions and maybe even blocks that contain sanctioned hashes.

Powerful sanctions regimes provide lists of sanctioned hashes for all major blockchains, including Bitcoin. These hashes end up being incorporated into output script templates that reside on the Bitcoin blockchain, and when these hashes are included, transactions need to be rejected.

### Is yo
...

💬 dergoegge commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#issuecomment-1936203492)
> might be good to recap why it was only added in BLOCK processing but not other ProcessBlocks: In every other case we already don't punish the sender of compact blocks for failing these higher level checks, while full blocks allow for punishment.

We call `ProcessBlock` when we receive a `block` message (handled in this PR) or as the result of a compact block reconstruction. Compact blocks are relayed before full validation occurs, therefore we don't punish peers for sending us invalid blocks
...

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484531064)
should cover the missing case

```suggestion
BOOST_CHECK_EQUAL(*PackageV3Checks(tx_v2_from_v3, GetVirtualTransactionSize(*tx_v2_from_v3), package_v3_v2, empty_ancestors), expected_error_str);
BOOST_CHECK_EQUAL(*PackageV3Checks(tx_v2_from_v3, GetVirtualTransactionSize(*tx_v2_from_v3), {tx_v2_from_v3}, *ancestors_v2_from_v3), expected_error_str);
```

🤔 instagibbs reviewed a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1872752668)
looks like if I revert TX_MAX_STANDARD_VERSION to 3, all unit/functional tests pass?

ACK 29029df5c700e6940c712028303761d91ae15847 modulo that

💬 instagibbs commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1484551682)
should we be checking PoW first before doing potentially a lot of hashing?

💬 jonatack commented on issue "Enable sanctions enforcement in Spam Filter Code":
(https://github.com/bitcoin/bitcoin/issues/29416#issuecomment-1936268247)
You are free to add this feature to your own fork of this open source software, but barring unforeseen circumstances, it won't achieve consensus and be merged in this project. Like your previous issue https://github.com/bitcoin/bitcoin/issues/29137, this will probably be closed as not planned.

💬 fjahr commented on pull request "test: Fix utxo set hash serialisation signedness":
(https://github.com/bitcoin/bitcoin/pull/29399#issuecomment-1936273179)
utACK fa0ceae970242d8d6bdef150c98f04c67b06e20c

💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1936304636)
re: https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1926350798

Thanks ishaanam for pointing out the lines of code that answer my questions. Since original links got broken in rebase, here are stable links that point to the two lines of code which prioritize the abandoned state over the conflicted state:

- https://github.com/bitcoin/bitcoin/commit/3de89342dcd44bdde4027a91cb27793dc0231847#diff-1f2db0e4d5c12d109c7f0962333c245b49b696cb39ff432da048e9d6c08944d8L1313-R1313
- https://g
...

💬 0xB10C commented on issue "Enable sanctions enforcement in Spam Filter Code":
(https://github.com/bitcoin/bitcoin/issues/29416#issuecomment-1936306770)
> `-sanctionblocks` : Enforce sanctions by rejecting new blocks that contain sanctioned transactions

I guess you're aware that anyone using that would hard fork off of Bitcoin with such an option. A few questions for you to consider: What happens when someone is doing IBD with this option enabled? Would they hard fork at the same height as the other nodes on the network? What happens when a part of the network updates their sanctioned list while another part of the network hasn't yet updated?
...

💬 furszy commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1484606348)
> Any reason to add __func__ to this log, when it previously wasn't there?

just an old bad habit. Removed. Thanks.