Skip to content

fix: four silent-wrong-result bugs in render, build, and network create - #208

Merged
kuny0707 merged 4 commits into
tronprotocol:developfrom
barbatos2011:fix/render-build-network-correctness
Aug 5, 2026
Merged

fix: four silent-wrong-result bugs in render, build, and network create#208
kuny0707 merged 4 commits into
tronprotocol:developfrom
barbatos2011:fix/render-build-network-correctness

Conversation

@barbatos2011

@barbatos2011 barbatos2011 commented Aug 5, 2026

Copy link
Copy Markdown

Four correctness fixes across the render, build, and deploy paths. They were found while trying to stand up a two-node private network built from a specific java-tron commit — every one of them had the same shape: the command succeeded and produced a wrong result. No error, no warning, and in three of the four cases a green config validate.

Rebased on fbb3658. make test -race, make lint, and make e2e (real Docker) all pass.


1. config_overrides rendered Go syntax, not HOCON — fix(render)

hoconValue fell back to fmt outside the scalar cases, and Go syntax is only accidentally JSON:

[]any{map[…]}    %v  ->  [map[address:T… voteCount:5000]]   no HOCON parser accepts this
"ctrl\x01char"   %q  ->  a Go \x-escape, which is not a JSON/HOCON escape

The first made every list-valued override unusable. Most visibly it made genesis.block.witnesses unexpressible, so the two-SR DPoS private network tron-docker documents could not be built from an intent file at all.

Both now render through a JSON encoder — HOCON is a JSON superset, so the encoder is the correct renderer. SetEscapeHTML(false) keeps URLs readable. Numbers deliberately stay on fmt; a test pins that so the next refactor has to change it on purpose.

2. build.revision labelled the artifact without building it — fix(build)

The git worktree checkout ran only when build.patches was non-empty. With an explicit branch/tag/sha and no patches, gradle ran in the caller's working tree — and then the resolved sha went into the cache key, the manifest and status.build_revision.

So two trond build --revision <ref> runs against different refs could return byte-identical artifacts under two different labels. Anyone using this to compare a base commit against a head commit would be comparing an artifact with itself, and the tooling would confirm the labels.

schema.go already documented the field as selecting "which git revision to build"; the code did the resolve half and skipped the build-that-commit half. revision: HEAD still builds the working tree with dirty edits — that is the dev inner loop, and the dirty state is already in the cache key.

3. network create bypassed apply.Applyfix(network)

The multi-node loop hand-rolled render + Deploy + state and had drifted from the core in three ways:

  • it never imported internal/build, so a node with build: rendered an empty image: and deployed nothing usable
  • it hardcoded jdk=17 instead of probing the target
  • it hardcoded the docker runtime instead of honouring target.runtime

Every node now goes through Apply, projected to a single-node intent with its own name and hash so idempotency stays per node.

Two things had to be fixed in Apply first, both found by doing the refactor rather than by reading it:

  • Apply never wrote P2PPort. network add builds the joining node's peer list from the P2PPort of every node in state and skips zero entries. Routing create through Apply as-is would have left every node invisible as a peer, so a late joiner would come up with an empty peer list and never connect — with nothing in either command's output to say why.
  • Monitoring would have deployed once per node. RenderHOCON keys its metrics auto-enable off Intent.Monitoring, so the field cannot simply be cleared. Options.SkipMonitoring suppresses the deploy and leaves the field alone. Without it the last node's stack wins and scrapes exactly one node — a monitoring setup that looks healthy and observes a fraction of the network.

Two consequences of create becoming an Apply caller, both wanted: it inherits RenderHOCONWithSecrets (#202) instead of keeping a second copy of that call in sync by hand, and it now passes guard.Requested() so the state-based half of the --require-private gate (#203) applies here too.

4. jvm.extra_optsfeat(intent)

JVMConfig was closed over heap and GC. java-tron ships operational flags in gradle/java-tron.vmoptions, read by its distribution launcher bin/FullNode; trond runs the JAR directly, so that file never applies and there was no way to supply them.

-Dio.netty.allocator.type=pooled is the case that forced this — java-tron sets it to opt out of netty 4.2's adaptive allocator, which has open direct-memory regressions upstream. Deployed through trond, a node ran on the allocator java-tron had explicitly avoided.

Appended last, because the JVM takes the final occurrence of -XX:± and -D — appending earlier would make the field look wired while trond's own flag quietly won. A test pins the ordering.

Validation is an allowlist by construction: only -D<key>=<value> and -XX:…, so -javaagent / -agentlib / -cp / @argfile are excluded by shape rather than by blocklist, and a JVM flag added upstream tomorrow cannot slip in. Whitespace is refused, and that rule is load-bearing rather than tidiness: JVMArgs joins with spaces, so "-Dfoo=bar -XX:+Evil" in one list item reads as an innocuous property in the intent and arrives as two JVM arguments.


Testing

  • make test -race — 25 packages, 0 failures
  • make lint — no findings
  • make e2e — full suite against a real Docker daemon, including TestE2E_Network_PrivateLifecycle (2 containers, create → status → destroy)
  • New tests use the property as the oracle where possible: rendered HOCON values are parsed back rather than string-compared; the build tests assert the contents of the tree gradle saw, not the label on the artifact

One note for reviewers: the HOCON tests deliberately do not parse the whole rendered document. gurkankaymak/hocon cannot parse any of the shipped java-tron templates — private fails at 112:22, mainnet at 37:5, nile at 103:5 — even before an override is appended. Typesafe Config, which java-tron actually uses, accepts them. The tests therefore parse the rendered value.

Not covered

  • network create --monitor is unit-tested but not exercised with real containers
  • target.type: ssh on the multi-node path is untested

hoconValue used fmt for everything outside the scalar cases, and Go
syntax is only accidentally JSON. Two separate defects fell out of that:

  []any{map[…]}   %v -> [map[address:T… voteCount:5000]]   HOCON rejects
  "ctrl\x01char"  %q -> a Go \x-escape, not a JSON \u-escape

The first made every list-valued override unusable. Most visibly it made
genesis.block.witnesses unexpressible, so the two-SR DPoS private network
tron-docker documents could not be built from an intent file at all.

Route both through a JSON encoder — HOCON is a JSON superset, so the
encoder is the correct renderer. SetEscapeHTML(false) keeps URLs and &
readable instead of turning them into &.

Numbers stay on fmt on purpose: %v already yields JSON-compatible output
for every numeric type YAML produces (including the 1e+06 form the JSON
grammar allows), and moving them would change existing rendered configs
for no correctness gain. A test pins that so the next refactor has to
make the change deliberately.

Tests assert the property rather than the string: each value is parsed
back. Note the whole-document oracle is deliberately NOT used —
gurkankaymak/hocon cannot parse the shipped java-tron templates at all
(private fails at 112:22, mainnet 37:5, nile 103:5) even before an
override is appended; Typesafe Config, which java-tron actually uses,
accepts them. The end-to-end test therefore decodes the rendered value.
…ng with it

setupWorktree was gated on len(Patches) > 0, so `revision: <sha>` with no
patches never checked that revision out. Gradle ran in the caller's
working tree while the resolved sha went into the cache key, the manifest
and status.build_revision.

The failure mode is the bad kind: not an error, a confidently mislabelled
artifact. Building base and head from one checkout could produce identical
bytes under two labels, so a comparison intended to isolate a change would
compare an artifact against itself and report no difference.

schema.go already documents the field as selecting "which git revision to
build" and says branch/tag/sha "resolve to that exact commit" — the code
did the resolve and skipped the build-that-commit half. Widen the trigger
to cover an explicit non-HEAD revision, patches optional.

HEAD keeps building the working tree deliberately: that is the dev inner
loop, and Source.Resolve already folds dirty state (including untracked
files) into the cache key for exactly that case. Tests pin both halves so
neither can be "simplified" into the other.

Also splits the failure code — PATCH_FAILED is misleading when the caller
declared no patches and the worktree setup itself failed.
The multi-node path hand-rolled render + Deploy + state and had drifted
from the core it was meant to mirror:

  - it never imported internal/build, so a node with `build:` rendered an
    empty image: line and deployed nothing usable — silently, with a green
    config validate
  - it hardcoded jdk=17 for JVM arg selection instead of probing the target
  - it hardcoded the docker runtime instead of honouring target.runtime

nodeIntent projects the network intent down to the single-node form Apply
consumes. Apply keys the compose project, the state entry and Result.Name
off Intent.Name and reads only Nodes[0], so the projection renames as well
as slices. The hash is computed over the projected intent, not the network
file, so editing one node redeploys that node and leaves its siblings at
no_change — a shared hash would make node 1 look unchanged the moment node
0 had been applied.

Two things had to be fixed in Apply first, both found by doing the
refactor rather than by reading it:

  - Apply never wrote P2PPort. network add builds the joining node's peer
    list from the P2PPort of every node in state and SKIPS zero entries, so
    routing create through Apply as-is would have left every node
    unreachable as a peer and the late joiner permanently isolated — with
    nothing in either command's output to say why.

  - monitoring would have been deployed once per node. RenderHOCON keys the
    metrics auto-enable off Intent.Monitoring so the field cannot simply be
    cleared; SkipMonitoring suppresses the deploy and leaves the field
    alone. Without it the last node's stack wins and scrapes exactly one
    node, which looks healthy and is not.

Two consequences of create becoming an Apply caller, both wanted:

  - it inherits RenderHOCONWithSecrets (tronprotocol#202), so the witness key is no
    longer inlined by a second copy of that call kept in sync by hand
  - it now passes guard.Requested() as RequirePrivate, so the state-based
    half of the gate (tronprotocol#203) applies here too. guard.Enforce above only
    sees the intent's network LABEL; the core also checks the network
    recorded in state for a node already deployed under the same name.
    Restores "network create" to the list of callers that inherit it.

Validated against a real Docker daemon: make e2e passes, including
TestE2E_Network_PrivateLifecycle (2 containers, create → status → destroy).
… derive

JVMConfig was closed over heap and GC, which is most tuning but not all of
it. java-tron ships operational flags in gradle/java-tron.vmoptions, read
by its distribution launcher bin/FullNode. trond runs the JAR directly, so
that file never applies and there was no way to supply those flags.

-Dio.netty.allocator.type=pooled is the case that forced this: java-tron
sets it to opt out of netty 4.2's adaptive allocator, which has open
direct-memory regressions upstream. Deployed through trond, a node ran on
the allocator java-tron had explicitly avoided, and no intent could say
otherwise.

Appended LAST, after everything trond derives, because the JVM takes the
final occurrence of -XX:± and -D — appending earlier would make the field
look wired while trond's own flag quietly won. A test pins the ordering.

Validation is an allowlist by construction rather than a blocklist: only
-D<key>=<value> and -XX:… are accepted, so the flags that load code or
rewrite the classpath (-javaagent, -agentlib, -cp, @argfile) are out by
shape and a JVM flag added upstream tomorrow cannot slip in.

Whitespace is refused, and that rule is load-bearing, not tidiness:
JVMArgs joins the set with spaces, so an entry holding one does not stay a
single argument — "-Dfoo=bar -XX:+Evil" in one list item would read as an
innocuous property in the intent and arrive as two JVM arguments. Quoting
characters are refused for the same reason on the other side: these land
in a compose command list entry and a systemd ExecStart line.

schemas/intent.schema.json is updated in the same commit — it declares
additionalProperties: false, so without it the Go side would accept a
field that schema validation rejects.
@barbatos2011
barbatos2011 force-pushed the fix/render-build-network-correctness branch from ddea6c3 to 7942749 Compare August 5, 2026 08:56
@kuny0707
kuny0707 merged commit cd86916 into tronprotocol:develop Aug 5, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants