Conversation
Openapi-ai.yaml removed
Enforce chainSlice range limit
Validate wallet scan id route parameters
Validate REST modifier id byte length
…e-id-length Validate Transaction API box and token id lengths
…-hex Reject invalid secret proof hint hex
Add Inv -> RequestModifier test to ErgoNodeViewSynchronizerSpecificat…
Remove incorrect security annotations from public mining and script API routes
Preserve asset issuance with token burn requests
Bound per-peer outbound buffering
…-cleanup Close all live connections for blacklisted IPs
…-ids Prevent duplicate IDs in OrderedTxPool
Fix indentation in openapi.yaml for CommitmentWithSecret
Block candidate generation improvements
Preserve extra index consistency across chain switches
Log method, relative URI, response status and elapsed time for every query served by the node's HTTP interface. Bodies are not logged: requests to this API carry secrets (mnemonic on /wallet/restore, password on /wallet/unlock). Logging goes through ScorexLogging rather than akka's LoggingAdapter, so no dependency is added and the node's HTTP verbosity is not tied to akka's global log level. It is off by default, as the root logger is at INFO, and costs nothing when off since log.debug is a macro guarded by isDebugEnabled. The directive wraps the route outside handleRejections, so rejected requests are logged too, with the status they were answered with. Closes #1909
Commented-out logger element so the switch is discoverable.
Attaches a logback ListAppender to the service logger and asserts on what is emitted: one line per served query with method, URI, status and duration; the query string included and unmatched paths logged with the status they were answered with; nothing logged below DEBUG; and the response body unchanged with logging on and off.
Log API queries at DEBUG level
jozanek
left a comment
jozanek
left a comment
There was a problem hiding this comment.
Reviewed the full branch against master, focused on protocol/wire compatibility, concurrency, DoS surface, and test coverage.
No protocol blockers: serializers untouched, stricter NiPoPoW validation is bootstrap-only with prover/verifier symmetry intact, FullBlockApplied.txIds never hits the wire, and the outbound buffer cap changes no message format. Test coverage is excellent — every fix ships a regression spec.
Findings below: 3 minor, 5 nit — release-note/visibility items, no code defects.
| * | ||
| */ | ||
| case class PoPowParams(m: Int, k: Int, continuous: Boolean) | ||
| final class PoPowParams private (val m: Int, val k: Int, val continuous: Boolean, val minChainLength: Int) |
There was a problem hiding this comment.
MINOR: PoPowParams going from case class to a private-constructor class with apply returning Try is a source/binary break in ergo-core's public API (no more direct construction, copy, or unapply). Since ergo-core is the library SPV clients build against, this deserves an explicit entry in the 6.0.5 release notes.
|
|
||
| def apply(m: Int, k: Int, continuous: Boolean): Try[PoPowParams] = Try { | ||
| require(isValid(m, k), s"Invalid NiPoPoW parameters: m=$m, k=$k") | ||
| new PoPowParams(m, k, continuous, m + k) |
There was a problem hiding this comment.
NIT: minChainLength is computed and stored but never read in production code (only one test asserts it). Either drop it or use it in prove's chain.lengthCompare(k + m) check so it earns its place.
|
|
||
| private val mempoolCapacity = settings.nodeSettings.mempoolCapacity | ||
|
|
||
| private def withoutTransaction(id: ModifierId): TreeMap[WeightedTxId, UnconfirmedTransaction] = { |
There was a problem hiding this comment.
MINOR: the self-heal fallbacks (withoutTransaction's full filter, hasUnregisteredTransaction, currentTransaction's orElse scan) are O(n) per mutation once orderedTransactions.size != transactionsRegistry.size, so a corrupted pool under tx flood pays O(n) per admission until healed. The healthy path keeps O(log n) via the size-equality guard, so this is fine as a recovery path — but consider logging when the degraded path triggers, so pool corruption is visible in production instead of silently costing CPU.
| val elapsed = System.currentTimeMillis() - stats.startMeasurement | ||
| if (stats.takenTxns != 0) { | ||
| elapsed * posInPool / stats.takenTxns | ||
| val cappedElapsed = math.max(0L, math.min(elapsed, MemPoolStatistics.measurementIntervalMsec.toLong)) |
There was a problem hiding this comment.
NIT: MemPoolStatistics.measurementIntervalMsec = 60 * 1000 is commented "one hour" but is one minute. Pre-existing, but the new elapsed-time cap here now depends on this constant, so worth fixing the comment while in the area.
| MaxMessageSize.toLong + HeaderLength + ChecksumLength | ||
|
|
||
| // Independently bound collection overhead from small messages. | ||
| private[network] val MaxBufferedOutboundMessages: Int = 64 |
There was a problem hiding this comment.
NIT: MaxBufferedOutboundMessages = 64 and the byte cap are hard-coded. If field tuning ever turns out to be needed (e.g. peers on high-latency links tripping the abort), exposing them under scorex.network would avoid a redeploy — fine to defer.
| val modifierIdGet: Directive1[ModifierId] = parameters("id".as[String]) | ||
| .flatMap(handleModifierId) | ||
|
|
||
| private def parseModifierId(value: String): Try[ModifierId] = |
There was a problem hiding this comment.
MINOR: client-visible behavior change worth a release-note entry: modifier/box/token ids with wrong byte length and scan ids outside Short range now return 400 where they were previously accepted, silently truncated (scanIdInt.toShort querying the wrong scan!), or 500'd. Good hardening — just make sure API consumers hear about the stricter validation.
| } ~ | ||
| (path("openapi.yaml") & get) { | ||
| getFromResource("api/openapi-ai.yaml", ContentTypes.`text/plain(UTF-8)`) | ||
| } |
There was a problem hiding this comment.
NIT: removal of /openapi.yaml and /.well-known/ai-plugin.json is a deliberate feature removal, but anyone who scripted against those endpoints will notice — one line in the release notes would cover it.
| .withFallback(nodeSeedConfigs.head) | ||
| .withFallback(allowLocalConfig) | ||
|
|
||
| // `lazy` so the container is only started when a test actually touches `node`. |
There was a problem hiding this comment.
NIT: the lazy val change is right, but it documents that the only OpenAPI conformance test remains ignored (checker image gone) — so the openapi.yaml edits on this branch aren't machine-checked. Worth a tracking issue to restore an OpenAPI validation step.
jozanek
left a comment
jozanek
left a comment
There was a problem hiding this comment.
Review of the 6.0.5 release candidate
What "Request changes" means here: GitHub will show this PR as blocked on this review until it is re-reviewed or dismissed. I am using it because of one MAJOR functional finding — the extra indexer can stall permanently after the new catch-up deferral (inline below) — which should be fixed, or explicitly accepted, before the release is tagged. It is not a protocol or consensus objection.
Scope: the full PR diff against master — mempool duplicate-id fix, candidate-generator improvements, extra-indexer reorg handling, API input validation and openapi corrections, AI-plugin removal, NiPoPoW parameter/PoW validation, p2p outbound-buffer cap and blacklist cleanup, wallet burn-order fix, mempool fee/wait-time clamps, and the API query logger.
Protocol screen — why there are no blockers:
- No serializer byte-format changes anywhere. The NiPoPoW hardening is verifier-side only: the deserializer still parses the same byte shapes (the new tests round-trip proofs with
m = 0and onlyisValidrejects them), and the tightenedisValid(param sanity + per-header Autolykos PoW) only rejects proofs an honest prover never produces, so prover/verifier symmetry is retained. LocalBlockApplied/RemoteBlockAppliedgained atxIdsfield, but these are internal event-stream messages published byErgoNodeViewHolder; they are never serialized to the network.- The
appVersion = 6.0.5handshake bump is the standard release procedure, and the openapi/do-release.shversion stamps are consistent with it. - The outbound-buffer cap and blacklist changes alter connection management only, not the wire format.
Also verified along the way: the removed openapi security annotations now match the code (only candidateWithTxs carries withAuth in MiningApiRoute; ScriptApiRoute has none), the old chainSlice range guard was dead code so the new check closes a real unbounded-request hole, and the scan-id .toShort truncation fix stops queries like 70000 from silently returning scan 4464's data.
Findings: 1 MAJOR, 5 MINOR, 3 NIT — all inline.
| context.become(receive.orElse(loaded(newState))) | ||
| self ! Index() | ||
| } else { | ||
| log.info("Deferring catch-up because the next header does not extend the indexed tip") |
There was a problem hiding this comment.
MAJOR The deferral branch stops the Index() self-loop without scheduling any retry, and while caughtUp = false the actor has no handler for FullBlockApplied (the handler at line 501 requires caughtUp), so the only signal that can resume indexing is a Rollback event.
Consider a near-tip headers-only fork that briefly becomes the best header chain but whose full blocks never win (the losing side of a miner race): the guard at line 481 sees a next header that does not extend the indexed tip and defers. If the original chain then outgrows the fork, the best header chain flips back — but no Rollback is ever published, because the full-block chain never switched. The indexer stays stalled until node restart, silently dropping every subsequent FullBlockApplied.
Suggestion: add case _: FullBlockApplied if !state.caughtUp && !state.rollbackInProgress => self ! Index() so every applied block re-evaluates the deferral condition (or re-schedule Index() with a short delay instead of only logging).
| if (modCount >= saveLimit) saveProgress(newState) | ||
| context.become(receive.orElse(loaded(newState))) | ||
| self ! Index() | ||
| val nextHeaderOpt = history.bestHeaderAtHeight(state.indexedHeight + 1) |
There was a problem hiding this comment.
MINOR During catch-up this adds a bestHeaderAtHeight(h + 1) (heightIds index read + full header fetch) per block, and index() then re-reads bestHeaderIdAtHeight(height) at line 381 because headerOpt is None on this path — two redundant storage reads per block on the full-reindex hot path, where they multiply across millions of blocks.
Suggestion: pass the already-fetched nextHeaderOpt into index(state.incrementIndexedHeight, nextHeaderOpt) so both the parent check and indexedHeaderId reuse a single read.
| )) | ||
| }) | ||
| } ~ | ||
| (path(".well-known" / "ai-plugin.json") & get) { |
There was a problem hiding this comment.
MINOR Removing /openapi.yaml and /.well-known/ai-plugin.json is clearly intentional (ChatGPT-plugin retirement), but it is a breaking removal of public endpoints — anything still fetching them gets a 404 after upgrade. Worth an explicit line in the 6.0.5 release notes.
| // `lazy` so the container is only started when a test actually touches `node`. | ||
| // The single test below is currently `ignore`d (the openapi-checker image is gone), | ||
| // so without `lazy` we would start and tear down a node for nothing. | ||
| lazy val node: Node = docker.startDevNetNode(offlineGeneratingPeer).get |
There was a problem hiding this comment.
MINOR The spec's only test remains ignored (the openapi-checker image is gone), so this suite passes CI while providing zero signal — the new comment documents the situation but keeps the dead spec. Consider deleting the spec or reviving the check with a maintained validator image.
| * | ||
| */ | ||
| case class PoPowParams(m: Int, k: Int, continuous: Boolean) | ||
| final class PoPowParams private (val m: Int, val k: Int, val continuous: Boolean, val minChainLength: Int) |
There was a problem hiding this comment.
MINOR minChainLength is not read anywhere in production code — its only consumer is the assertion in PoPowAlgosSpec. If it is groundwork for the follow-up NiPoPoW parsing work (#2461), fine to keep, but then a short comment saying so would help; otherwise it is a dead field that suggests a validation which does not actually happen yet.
| suffixHead.checkInterlinksProof() | ||
| } | ||
|
|
||
| lazy val hasValidPow: Boolean = headersChain.forall(popowAlgos.hasValidPow) |
There was a problem hiding this comment.
MINOR Validating every header's Autolykos PoW is the right fix, but isValid is evaluated inside ErgoNodeViewSynchronizer's receive (case Success(proof) if proof.isValid around line 1085 of ErgoNodeViewSynchronizer.scala), so a proof chain of hundreds of headers now runs full PoW verification on the synchronizer's dispatcher thread, stalling its mailbox during nipopow bootstrap — and several proofs can arrive back-to-back from the p2pNipopows peers.
Suggestion: run the proof validation in a Future on a dedicated dispatcher and pipeTo the result back, keeping the synchronizer responsive.
| private val mempoolCapacity = settings.nodeSettings.mempoolCapacity | ||
|
|
||
| private def withoutTransaction(id: ModifierId): TreeMap[WeightedTxId, UnconfirmedTransaction] = { | ||
| // Keep healthy mutations logarithmic; scan by ID only after cardinality diverges. |
There was a problem hiding this comment.
NIT The size-equality heuristic takes the fast path on compensating corruption — one duplicate key plus one orphaned entry leaves the sizes equal, so a duplicate would survive withoutTransaction. Fine as best-effort self-healing, but worth extending the comment to note that limitation.
| done.await() | ||
| awaitCondition(done) | ||
| indexer ! GenerateBetterChainTip() | ||
| lock.lock() |
There was a problem hiding this comment.
NIT awaitCondition fixes the lock discipline (the old pattern never unlocks after await()), but this test still uses bare lock.lock(); created.await() in two places. Worth finishing the migration to awaitCondition(created) here too.
|
|
||
| // Keep one maximum serialized frame per peer. Backpressured snapshot transfers | ||
| // retry instead of retaining their entire application-level in-flight window. | ||
| private[network] val MaxBufferedOutboundBytes: Long = |
There was a problem hiding this comment.
NIT Question on honest-path headroom: the byte cap is one max frame (~16.4 MB), which the tests show fits 4 in-flight snapshot chunks — but a peer that requested a large block batch can legitimately have several Modifiers responses (up to ~8.4 MB each) queued while its socket is stalled, and two of those already exceed the cap, aborting the connection. Reconnect makes this self-healing, so it may well be acceptable — but worth confirming the serving side never queues more than one large response per request round, or noting that connection churn under full-stall is the intended trade-off.
6 participants
Uh oh!
There was an error while loading. Please reload this page.