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

🤔 furszy reviewed a pull request: "wallet: simplify and batch zap wallet txes process"
(https://github.com/bitcoin/bitcoin/pull/28987#pullrequestreview-1872874268)
Updated per feedback. Thanks Marko and achow.

💬 furszy commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1484621811)
> I'm not sure if `NONCRITICAL_ERROR` is really the right return code. It seems like something has gone critically wrong if `TxnCommit` were to fail.

The returned error code doesn't really matter. Anything different from `LOAD_OK` is an error for all the function's callers. And they behave exactly the same for all codes.
Actually, the `DBErrors` return doesn't make sense anymore after this PR changes. The function is no longer traversing the entire db nor checking for extra, unneeded, stuff
...

💬 furszy commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1484606675)
> This should explicitly call TxnAbort before returning as otherwise we will get extraenous logs about the tx being aborted before batch is destroyed.

sure. Fixed.

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484624000)
Done

🤔 TheCharlatan reviewed a pull request: "RFC: build: remove confusing and inconsistent disable-asm option"
(https://github.com/bitcoin/bitcoin/pull/29407#pullrequestreview-1872906747)
Do the mentiones from the `doc/fuzzing.md` to `--disable-asm` still need to be removed? Seems like `--disable-asm` is not recognized by libsecp, it supports a package-specific asm option.

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484625047)
Yes, I've added a comment for that.

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484626857)
I have added a lot more comments to the test which should clarify this.

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484632206)
I have added a comment before all of the tests in this file explaining how these tests create conflicts.