Feat/signers send failed txs to miner

[jacinta-stacks]

Closes #6659
Closes #6660

• Added failed_txid field to block validation reject responses and signer rejection messages, allowing signers to report which specific transaction caused a block proposal to fail validation
• Introduced two-tier handling: BadTransaction rejections exclude the tx until the next successful block production (transient failures like deadline exceeded), while the new ProblematicTransaction reject code permanently blacklists the tx from the mempool (genuinely broken txs like parse errors, Clarity crashes, etc.). I could also change it to be banned for just the tenure or something if that is too harsh a punishment. I am not 100% sure if ProcessingFailure is transitory or indicates a bigger problem...but I treat those as only temporary failures.
• Requires a blocking minority threshold (>30% of signer weight) before acting on any failed_txid

[codecov]

Codecov Report

❌ Patch coverage is 65.58442% with 53 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.93%. Comparing base (995ebd0) to head (16c9d3c).

Files with missing lines	Patch %	Lines
stackslib/src/net/api/postblock_proposal.rs	21.95%	32 Missing ⚠️
libsigner/src/v0/messages.rs	68.96%	9 Missing ⚠️
...tacks-node/src/nakamoto_node/stackerdb_listener.rs	61.90%	8 Missing ⚠️
stacks-node/src/nakamoto_node/miner.rs	94.11%	2 Missing ⚠️
...tacks-node/src/nakamoto_node/signer_coordinator.rs	86.66%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #6964      +/-   ##
===========================================
- Coverage    68.63%   64.93%   -3.71%     
===========================================
  Files          412      412              
  Lines       219013   219145     +132     
  Branches       338      338              
===========================================
- Hits        150330   142306    -8024     
- Misses       68683    76839    +8156     

Files with missing lines	Coverage Δ
stacks-node/src/nakamoto_node.rs	87.50% <ø> (ø)
stackslib/src/chainstate/stacks/miner.rs	79.71% <100.00%> (+6.30%)	⬆️
stackslib/src/config/mod.rs	59.52% <100.00%> (+2.24%)	⬆️
stacks-node/src/nakamoto_node/miner.rs	80.13% <94.11%> (+5.31%)	⬆️
...tacks-node/src/nakamoto_node/signer_coordinator.rs	80.15% <86.66%> (-0.61%)	⬇️
...tacks-node/src/nakamoto_node/stackerdb_listener.rs	85.36% <61.90%> (+4.66%)	⬆️
libsigner/src/v0/messages.rs	63.68% <68.96%> (+2.03%)	⬆️
stackslib/src/net/api/postblock_proposal.rs	74.58% <21.95%> (+7.70%)	⬆️

... and 269 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 995ebd0...16c9d3c. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[brice-stacks]

Looks good. Two minor comments and one concern remaining.

[brice-stacks]

lgtm

[brice-stacks]

I resolved the merge conflicts in this one. It would be nice to get this released soon.

[coveralls]

Coverage Report for CI Build 24350271522

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage decreased (-0.02%) to 85.697%

Details

• Coverage decreased (-0.02%) from the base build.
• Patch coverage: 36 uncovered changes across 4 files (134 of 170 lines covered, 78.82%).
• 1814 coverage regressions across 61 files.

Uncovered Changes

File	Changed	Covered	%
stackslib/src/net/api/postblock_proposal.rs	44	14	31.82%
libsigner/src/v0/messages.rs	44	41	93.18%
stacks-node/src/nakamoto_node/miner.rs	32	30	93.75%
stacks-node/src/nakamoto_node/signer_coordinator.rs	15	14	93.33%

Coverage Regressions

1814 previously-covered lines in 61 files lost coverage.

Top 10 Files by Coverage Loss	Lines Losing Coverage	Coverage
stackslib/src/net/inv/epoch2x.rs	226	79.21%
stackslib/src/net/chat.rs	200	92.93%
stacks-node/src/nakamoto_node/miner.rs	132	87.34%
stackslib/src/clarity_vm/database/marf.rs	99	60.67%
stackslib/src/net/api/postblock_proposal.rs	98	80.14%
clarity/src/vm/contexts.rs	75	92.36%
clarity/src/vm/functions/database.rs	75	92.36%
stackslib/src/net/codec.rs	69	91.63%
stackslib/src/net/p2p.rs	69	74.1%
stackslib/src/clarity_vm/database/ephemeral.rs	61	56.82%

---

Coverage Stats

Relevant Lines:	218012
Covered Lines:	186830
Line Coverage:	85.7%
Coverage Strength:	17219218.96 hits per line

---

💛 - Coveralls

[benjamin-stacks]

LGTM. I left various notes that I'd like to see addressed, but nothing that would block merging this PR, I'm happy to open follow-up PRs instead.

[jcnelson]

Unless I'm missing something, I think that, as written, there's a denial-of-service bug in this PR. If an attacker discovers a problematic transaction, they can iteratively spam the mempool with many high-fee variations of it and force chronic, repeated block rejections, and thereby stall the chain. The fix would be for the miner to try the proposal again with the exactly the same transactions as before, minus the problematic ones and their dependents (i.e. without querying the mempool). You could do this by setting the replay_transactions slice passed into NakamotoBlockBuilder::build_nakamoto_block(). In this PR, it appears that the miner recovers from a signer-rejected transaction by rebuilding a block from the mempool (instead of the already implicitly-approved remaining transactions), which allows the attacker to reliably slip in novel problematic transactions.

[benjamin-stacks]

Unless I'm missing something, I think that, as written, there's a denial-of-service bug in this PR

Is that really something that's coming from this PR? Wouldn't that work just as well today?

[jcnelson]

Is that really something that's coming from this PR? Wouldn't that work just as well today?

The status quo is that, if the signer determines that a block is invalid due to a problematic transaction, the miner would attempt to naively produce new blocks which could contain the same problematic transaction over and over. This behavior leads to a chain stall which can only be fixed if the miner refrains from including the problematic transaction.

This PR tries to fix this by making it so the miner node drops the signer-rejected problematic transactions from the mempool. This tactic isn't robust enough because the same problematic transaction could get re-inserted into the mempool on mempool sync or rebroadcast, leading to the status quo behavior. Also, if there is a large influx of problematic transactions, the current approach will still lead to a chain stall, since on re-attempt the miner will have a good attacker-controlled chance of picking a novel rejected transaction.

To fully address this, we need to do two things:

• The miner itself has to remember to ignore these transactions for the remainder of its tenure, so it won't re-exhibit the status quo behavior even if they get into the mempool again.

• The miner has to retry their block proposal using only known-good transactions (i.e. all the ones up to the one that was rejected, and others that are not causally-dependent). It shouldn't pull new ones out of the mempool until the block it proposes is either accepted, or rejected for some other reason.

[benjamin-stacks]

Right, and those are valid points, but they are points that weren't introduced by this PR.

This PR fixes one specific thing that it set out to fix: If there is a discrepancy between what the miner considers valid and what the signers consider valid (for whatever reason), that'll currently cause an unconditional stall because the miner doesn't know why the block was rejected, and thus re-proposes it over and over again. With this change, the miner has a chance to rectify things, because it now knows the reason for the rejection.

So there are stalls that might happen with the status quo but will not happen with this change.

On the other hand, unless I'm completely misunderstanding you, there are no stalls that happen with this change that would not have happened before this change.

If that's the case, this PR is a clear net positive. And it has previously had approvals from three different people (including yourself).

All I'm saying is that we should merge it, and if we want to improve things even further, we can create a follow-up ticket and a new PR. That way, we have the initial benefit of this PR right now, and the new PR is easier to review.

[brice-stacks]

I would agree with @benjamin-stacks's response above. I don't see how this creates any new problems, only that it doesn't solve all potential problems.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.