Input blocks: derive difficulty only from a parent bound to the best chain - #2552
cafebedouin wants to merge 2 commits into
Conversation
…chain Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Update, 17 September: sender-only correction checked. @cafebedouin, I ran both parent-validation suites on #2552 at The previously red timeout regression now passes. Both paths cover expiration without another-peer retry or non-delivery penalty, protection against an earlier attempt's expiration, and acceptance of a timely requested header. The two input-announcement replay properties pass too. Focused source review found no new issue in this increment. These are receiver/history tests, not full network or UTXO-application evidence. The unchanged input height+2 path and generic delivery-attempt hardening in #2528 remain separate. Review order: #2552 first, then #2553's remaining increment. #2553 contains #2552 in its ancestry. Both still target Hosted CI is still blocked before tests by the unavailable Sigma snapshot: #2552 run and #2553 run. This does not establish green CI or settle the maintainer decision on dropping versus retaining announcements with unknown/off-best-chain parents. Earlier evidence below refers to the previous heads. Its red timeout result and pending-variant status are superseded by the update above. Tested composition: weak-blocks
This clarifies my earlier recovery wording: best-header selection and best-full-block selection are distinct. These tests cover history selection and explicit receiver-side replay, relevant to #2506. Coverage stops at the synchronizer handoff; the #2506 sender and UTXO-state application are outside this patch. Waitlist policy remains a separate decision. Please also preserve request-attempt identity on expiration, the invariant in #2528, and the existing legitimate-response test when adding your variant. The patch changes only Commands: The second command ran three additional tests: two passed and the expiration regression failed. It must become green with the correction. CI update, 17 September: maintainer approval was granted and all three runs executed. Every failed job in #2551, #2552, and #2553 stopped during dependency resolution: #2551 was approved and merged into The unavailable snapshot already has a source-pinned CI bootstrap in #2501, isolated in 13352c1e (two CI files). That existing prerequisite can be reviewed independently of #2501's broader composition. It has not been added to these parent-validation PRs. Rerunning unchanged workflows will not restore the missing dependency. Hosted validation and the sender-only correction are separate outstanding gates; these local results are not a merge-ready claim. Three focused regression cases (one expected red until the request correction lands)diff --git a/src/test/scala/org/ergoplatform/network/InputBlockParentBindingSpec.scala b/src/test/scala/org/ergoplatform/network/InputBlockParentBindingSpec.scala
index d43fb133d..0bd3ca481 100644
--- a/src/test/scala/org/ergoplatform/network/InputBlockParentBindingSpec.scala
+++ b/src/test/scala/org/ergoplatform/network/InputBlockParentBindingSpec.scala
@@ -58,7 +58,9 @@ class InputBlockParentBindingSpec extends AnyPropSpec with Matchers with FileUti
* @param initialDifficultyHex overrides the test chain's difficulty (the local chain keeps it)
* @param applyLocalChain whether the generated chain is applied to the local history
*/
- class Fixture(initialDifficultyHex: Option[String] = None, applyLocalChain: Boolean = true) extends AkkaFixture {
+ class Fixture(initialDifficultyHex: Option[String] = None,
+ applyLocalChain: Boolean = true,
+ requestTimeout: FiniteDuration = 2.seconds) extends AkkaFixture {
implicit val ec: ExecutionContextExecutor = system.dispatcher
val historySettings: ErgoSettings = {
@@ -68,11 +70,11 @@ class InputBlockParentBindingSpec extends AnyPropSpec with Matchers with FileUti
private val cs = historySettings.chainSettings
val realPowScheme = new AutolykosPowScheme(cs.powScheme.k, cs.powScheme.n)
- // a short delivery timeout, so that what happens after it can be observed
+ // Expiration tests use a short timeout; replay tests keep unrelated deadlines outside their window.
val synchronizerSettings: ErgoSettings =
historySettings.copy(chainSettings = cs.copy(powScheme = realPowScheme),
scorexSettings = historySettings.scorexSettings.copy(
- network = historySettings.scorexSettings.network.copy(deliveryTimeout = 2.seconds)))
+ network = historySettings.scorexSettings.network.copy(deliveryTimeout = requestTimeout)))
val ncProbe = TestProbe("NetworkControllerProbe")
val viewHolderProbe = TestProbe("ViewHolderProbe")
@@ -226,6 +228,94 @@ class InputBlockParentBindingSpec extends AnyPropSpec with Matchers with FileUti
}
}
+ property("an input announcement is accepted on replay after its parent joins the best header chain") {
+ withFixture(new Fixture(requestTimeout = 30.seconds)) { f =>
+ val originalTip = f.hist.bestFullBlockOpt.get
+ val parent = nextBlock(Some(f.chain(1)), originalTip.blockTransactions.txs, defaultExtension)
+ parent.height shouldBe originalTip.height
+ f.hist.contains(parent.header) shouldBe false
+ val ib = announcement(f, parent.id, originalTip.height + 1, requiredNBitsAfter(f, parent.header))
+ ib.valid(f.realPowScheme, f.state.stateContext.currentParameters,
+ Some(requiredNBitsAfter(f, parent.header))) shouldBe true
+
+ f.process(ib)
+
+ viewHolderGotInputBlock(f) shouldBe false
+ val initialMessages = networkMessages(f)
+ headerRequests(initialMessages) shouldBe Seq(Seq(parent.id) -> SendToPeer(f.peer))
+ penalized(initialMessages) shouldBe false
+
+ f.hist.append(parent.header).get
+ f.hist.isInBestChain(parent.header) shouldBe false
+ val nextHeader = nextBlock(Some(parent), originalTip.blockTransactions.txs, defaultExtension).header
+ f.hist.append(nextHeader).get
+ f.hist.isInBestChain(parent.header) shouldBe true
+ f.hist.bestFullBlockIdOpt shouldBe Some(originalTip.id)
+ ib.header.height shouldBe f.hist.fullBlockHeight + 1
+
+ f.process(ib)
+
+ f.viewHolderProbe.expectMsg(ProcessInputBlock(ib, f.peer))
+ val replayMessages = networkMessages(f)
+ penalized(replayMessages) shouldBe false
+ headerRequests(replayMessages) shouldBe empty
+ }
+ }
+
+ property("an input announcement is accepted on replay after its parent becomes the best full block in history") {
+ withFixture(new Fixture(requestTimeout = 30.seconds)) { f =>
+ val originalTip = f.hist.bestFullBlockOpt.get
+ val parent = nextBlock(Some(originalTip), originalTip.blockTransactions.txs, defaultExtension)
+ f.hist.contains(parent.header) shouldBe false
+ val ib = announcement(f, parent.id, parent.height + 1, requiredNBitsAfter(f, parent.header))
+ ib.header.height shouldBe f.hist.fullBlockHeight + 2
+ ib.valid(f.realPowScheme, f.state.stateContext.currentParameters,
+ Some(requiredNBitsAfter(f, parent.header))) shouldBe true
+
+ f.process(ib)
+
+ viewHolderGotInputBlock(f) shouldBe false
+ val initialMessages = networkMessages(f)
+ headerRequests(initialMessages) shouldBe Seq(Seq(parent.id) -> SendToPeer(f.peer))
+ penalized(initialMessages) shouldBe false
+
+ applyChain(f.hist, Seq(parent))
+ f.hist.bestFullBlockIdOpt shouldBe Some(parent.id)
+ f.hist.isInBestChain(parent.header) shouldBe true
+ ib.header.height shouldBe f.hist.fullBlockHeight + 1
+
+ f.process(ib)
+
+ f.viewHolderProbe.expectMsg(ProcessInputBlock(ib, f.peer))
+ val replayMessages = networkMessages(f)
+ penalized(replayMessages) shouldBe false
+ headerRequests(replayMessages) shouldBe empty
+ }
+ }
+
+ property("unknown-parent discovery expires without requesting the header from another peer") {
+ withFixture { f =>
+ val otherPeer = f.initialize()
+ val unknownParent = bytesToId(Array.fill(32)(0x5a.toByte))
+ val tip = f.hist.bestFullBlockOpt.get.header
+ val ib = announcement(f, unknownParent, tip.height + 1, DifficultySerializer.encodeCompactBits(1))
+
+ f.process(ib)
+
+ // Observe two delivery deadlines, including a potential retry against the other peer.
+ val messages = f.ncProbe.receiveWhile(max = 5.seconds, idle = 5.seconds) { case m => m }
+ val requests = headerRequests(messages).filter(_._1.contains(unknownParent))
+ requests.map(_._2 == SendToPeer(f.peer)) shouldBe Seq(true)
+ messages.exists {
+ case p: PenalizePeer => p.address == otherPeer.connectionId.remoteAddress
+ case _ => false
+ } shouldBe false
+ f.deliveryTracker.getRequestedInfo(Header.modifierTypeId, unknownParent) shouldBe None
+ f.deliveryTracker.status(unknownParent, Header.modifierTypeId, Seq.empty) shouldBe ModifiersStatus.Unknown
+ viewHolderGotInputBlock(f) shouldBe false
+ }
+ }
+
property("input block whose parent is a known header outside the best chain is dropped") {
withFixture { f =>
val tip = f.hist.bestFullBlockOpt.get |
| } | ||
| } | ||
| log.info(s"Not processing input block $subBlockId: parent ${subBlockHeader.parentId} is not bound to the best chain") | ||
| return |
There was a problem hiding this comment.
Nothing: processInputBlock returns Unit, so the bare return only ends the method, like the three early returns above it (height gap, digest mode, already known input block). The announcement is not processed further.
…once The parent header of an unbound input block announcement is requested from the announcing peer with a tracked request that expires after the delivery timeout instead of running a delivery check, so no other peer is asked for a header only the sender claimed to know, and no peer is penalized for not delivering it. The expiration carries its own timer, the one the delivery tracker stores for the request, so an expiration queued for an earlier attempt does not clear a newer request for the same header. Tests: the three regression cases from the review of ergoplatform#2552 (two replay cases and the sender-only expiration), and one for an expiration from an earlier request attempt. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0158ircj1NFMWMqGsKmhmaVw
|
@a-shannon, pushed The expiration carries its own timer, the Tests: your three cases are included as posted, plus one where the header is requested twice and the first attempt's expiration, arriving after the second request, leaves the second request intact and the sender's delivery is still accepted. Removing the timer check makes that test fail. #2553 carries the same change for ordering block announcements and is now stacked on this branch, since both use the helper. |
|
Superseded by #2554, consolidated as suggested. |
…e best chain; request an unknown parent from the sender only Input block announcements (processInputBlock) and ordering block announcements (processOrderingBlockAnnouncement) are validated against the difficulty derived from their parent header, and only when that parent is known, is in the best header chain, and is one block below the announced header (at genesis height, the configured initial difficulty). The announced header's own nBits is never used. Policy for an announcement whose parent is unknown or not bound this way: it is not processed, stored or relayed, and there is no penalty. An unknown parent header is requested from the announcing peer only, once, with a tracked request that expires after the delivery timeout instead of running a delivery check (requestHeaderFromSenderOnly / SenderOnlyRequestExpired): no other peer is asked for a header only the sender claimed to know, and no peer is penalized for not delivering it. The expiration carries the timer the delivery tracker stores for the request, so an expiration queued for an earlier attempt does not clear a newer request for the same header. Tests: InputBlockParentBindingSpec and OrderingBlockAnnouncementParentCheckSpec, with real Autolykos proof-of-work; the three regression cases from the review of ergoplatform#2552 are included as posted. Supersedes ergoplatform#2552 and ergoplatform#2553. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0158ircj1NFMWMqGsKmhmaVw
Prepared with Claude (Anthropic): Claude Fable 5.1 and Claude Opus 5 found the issue; Claude Opus 5 wrote the patch and tests.
In
processInputBlock, an unknown parent givesexpectedNBits = None, sovalidchecks proof-of-work against the announcednBits. A known parent is used without checking its position. The todo atInputBlocksProcessor.scala:862anticipates this.Line numbers refer to
weak-blocksat c216c5b.Check. The parent is bound when its header is known, is in the best header chain, and
header.height == parent.height + 1. Only then isexpectedNBitsderived (requiredDifficultyAfter; at genesis height, the initial difficulty, as in header validation). A bound announcement failingvalidis penalized as before. Height gates are unchanged. The parent need not be the best full block, so the todo atErgoNodeViewSynchronizer.scala:1472(last 1-2 ordering blocks) stays open. An input block extending the best full block is processed as before.Policy. One commented branch,
if (expectedNBits.isEmpty). A known parent outside the best header chain also reaches it, so whether fork announcements are processed is decided there. Default: not processed, no penalty; an unknown parent header is requested from the sender withrequestBlockSection, guarded bystatus == Unknown, as in the height + 2 branch.Request trade-off. A tracked request is re-sent to other peers after the timeout, so an invented parent id can get those peers penalized for not delivering it (when no other modifier arrived in the meantime). An untracked request gets the honest sender's reply penalized as spam. The alternative, one tracked request to the sender only, expiring without a delivery check, was implemented and tested and can be provided.
Open question. Keep unbound input blocks, as the paper's disconnected waitlist (
papers/inputblocks/main.tex:366-374) andtodo: save input block?(ErgoNodeViewSynchronizer.scala:1537) suggest?Not covered. The
fullBlockHeight + 2branch (:1531-1545) requests a header with no proof-of-work check.Tests.
testOnly org.ergoplatform.network.InputBlockParentBindingSpec(real Autolykos proof-of-work; the default test configuration uses a fake scheme). 6 of 10 fail without the change.Merge notes. Applies cleanly on @a-shannon's #2367, #2368, #2500, #2502, #2503 and #2506. From a code reading of #2506: a replayed input tip one height above the receiver's best full block, whose ordering parent the receiver does not have or has outside its best header chain, reaches the policy branch and is dropped by default. Without this change it is processed, and when the parent is unknown, against the difficulty the tip itself declares. Replays between nodes on the same best full block are processed as before.