bug fix and improvements - #54
Merged
Merged
Conversation
…tive libraries accumulation
…e rate limiting beyond per-second limits
…alking its routing table
… the peer version Ed25519 derives its per-signature randomness internally, so an application-supplied nonce adds nothing to a signature. The nonce is only meaningful as the CryptoBox nonce, so it now exists exactly when a value has a recipient, and PeerInfo drops it entirely. Removing it from PeerInfo would have weakened node authentication: the node digest was SHA-256(peerId, nodeId, nonce), and without the nonce it becomes a constant function of the (peerId, nodeId) pair, and so a permanent bearer credential that the peer owner could staple onto every later version without the node taking part again. The digest now covers the fingerprint and the sequence number instead, which binds the attestation to one version of one peer instance and is stronger than the nonce was: a signature issued at one sequence number can no longer be replayed onto a later one. Both digests change, so every existing signature is invalid: this breaks the wire format in both directions and requires a clean install rather than a schema migration.
The alpha, k, replacements and concurrentTasks options were only half connected: the routing and task layers still read hardcoded constants, so a configured value had no effect. KademliaOptions is now read in exactly one place, KadNode.deploy(), which hands plain values to DHT; nothing below KadNode depends on the configuration type. DHT owns the effective configuration and validates it. Neither bad value fails loudly downstream: alpha below 1 makes Task.canDoRequest() permanently false, so a task never issues an RPC and, since iteration is driven only by call state changes, never completes; concurrentTasks below 1 makes TaskManager.isReady() permanently false, so every task queues forever. Both are silent hangs, so the constructor rejects them outright. KadContext keeps no state of its own and delegates every accessor to the DHT, so a value cannot go stale - the Vert.x context in particular is only assigned at deployment, which is after the context object is built. Test doubles use a protected no-DHT constructor and must override every accessor they use; anything missed throws rather than returning a wrong value. k and replacements are now separate. They previously shared a single constant, so the replacement cache silently inherited the bucket size. RoutingTable and KBucket require both explicitly - the constructors that supplied implementation defaults are gone, so a caller cannot get them by accident. KadConstants collects the four defaults and the fixed intervals and thresholds that are not configurable, so the values governing one node's behavior can be read and adjusted in one place instead of being spread across the routing, task and RPC layers. Rename concurrentQueries to concurrentTasks, including the configuration key: the ceiling governs TaskManager, and a PingRefreshTask is not a query. The option has not shipped yet, so no deployed configuration is affected. BosonVerticle.vertxContext() widens from protected to public, matching getVertx(), which was already public. This removes the DHT override that existed only so tests could reach the context. Tests now pass explicit non-default parameters rather than relying on the defaults, which is what actually exercises the wiring.
Several limits were expressed as multiples of k and had been tuned when k was 8, so raising it to 16 changed what they meant. The same heuristics would compound badly for a super node raising k further: lookups would cost proportionally more, the candidate queue would grow quadratically in CPU, and responses would outgrow the MTU. Each limit is now tied to the thing that actually constrains it. The lookup iteration budget becomes 2*k + max(k, alpha * 8) instead of 3*k. The 2*k term is not slack but a floor: ClosestSet.isEligible() requires insertAttemptsSinceTailModification to exceed the capacity, so convergence needs about k insert attempts to fill the closest set and k+1 more that fail to improve its tail. A budget below that floor makes every lookup terminate by exhaustion rather than convergence, and LookupTask.isDone() reports COMPLETED either way, so nothing surfaces the breakage. The remaining term covers the depth ramp and iterations lost to unanswered RPCs, which the old heuristic did not: at k=8 it left roughly three dead RPCs of margin, since a timed-out call burns an iteration for STALLED and another for TIMEOUT. This is why the budget is derived rather than configured. The candidate queue is capped at 128 regardless of k. ClosestCandidates.add re-sorts the whole queue on every insertion, so its CPU cost grows with the square of the size, on the event loop. The cap binds only from k=43 upward and still leaves at least 2k spares through k=64, where the extra entries would never be consulted anyway. The three lookup subclasses now size their local seed set from the same helper, rather than computing k*3 separately and being free to drift. PingRefreshTask sizes its queue from k + replacements rather than 2*k. That deque holds one bucket's main entries plus its replacement cache, which were a single constant until the two were separated; 2*k stopped describing anything real at that point. FIND_NODE, FIND_VALUE and FIND_PEER now return min(k, 16, whatever fits the packet budget) nodes per family. Tying the count to k coupled a routing property to a transport one: at k=16 a dual-family response was about 1690 bytes, over both Network.maxPacketSize budgets (1450 for IPv4, 1200 for IPv6), and a fragmented UDP datagram is lost entirely if any one fragment is lost. The budget is split across the families actually requested, since single-family responses were never the problem. At the default k this yields 16 for one family, 12 per family for a dual-family response over IPv4 and 9 over IPv6 - close to where Ethereum's discv4 lands under the same constraint. A node raising k no longer emits oversized packets. Every constant in KadConstants is now documented with what it controls, why the value was chosen against what other implementations use, the trade-off and its direction, how it behaves as k grows, and whether it is protocol, implementation policy, or transport-driven. The intent is that the next person changing one of these does not have to re-derive the reasoning first. Two things worth stating up front: DHT_UPDATE_INTERVAL is a polling granularity rather than a work rate, so changing it does not change how much maintenance traffic the node emits, and the two bootstrap thresholds form a deliberate two-tier scheme where the node self-bootstraps from its own routing table before falling back to the shared bootstrap servers.
The two thresholds that decide when a node re-bootstraps were absolute literals, 30 and 8, that had been calibrated when k was 8: 8 was exactly one bucket's worth of contacts, and 30 about four buckets. Raising k to 16 left the literals untouched, so they silently became half a bucket and under two buckets. The node grew more reluctant to repair a thinning routing table, and the band in which it self-bootstraps from contacts it already knows narrowed - pushing load onto the shared bootstrap servers that the two-tier scheme exists to protect. They are now 3*k and 1*k, which at k=8 reproduces the original 24/8 and so recovers the intent rather than inventing a new one. Scaling alone is not enough, because "enough contacts to operate" tracks k only up to a point: past some absolute number a node can route in every direction regardless of how large its buckets are. Left uncapped, a super node at k=64 would want 192 entries before it stopped bootstrapping, and in a network that never offers it that many it would re-bootstrap every BOOTSTRAP_MIN_INTERVAL indefinitely, since the retry has no backoff. Both thresholds are therefore capped: BOOTSTRAP_THRESHOLD_ENTRIES at 64, and the server-fallback tier at half that. Capping only one tier would have been worse than capping neither. The scheme depends on the server-fallback threshold sitting strictly below the bootstrap threshold; with a ceiling on one and not the other, the two collide at k=64, emptying the self-bootstrap band so that every bootstrap contacts the servers, and invert above it, at which point the routine 30-minute self-lookup starts contacting them on every fire - the exact thundering herd the split exists to prevent, appearing only at a k unlikely to be exercised in testing. Defining the fallback ceiling as half the other makes the relationship structural rather than coincidental, so it cannot drift if either is retuned; encoding it as two independent literals would repeat the mistake this change is fixing. Behavior at the default k=16 is unchanged: 48 and 16, with both ceilings inert. KademliaOptions now validates ranges rather than mere positivity, so a configuration that would put the node far outside the region these thresholds were reasoned about is rejected at load time rather than producing a node that technically runs. The accepted range for each parameter is named and lives in one place: the record's compact constructor and the Builder setters both validate through the same checks. Keeping a separate copy of the rule in the Builder is how the two immediately diverged - the record gained ranges while the Builder still tested only for positivity, so builder.k(2) was accepted at the call site and then threw from build(), far from the cause. Two tests pin this down, one for the upper bounds that nothing previously exercised, and one asserting that a value either constructor rejects is rejected by both.
Both bootstrap fill paths queued their lookups at the head of the task queue unconditionally. That is the right call exactly once: at startup the node has no routing table, cannot answer anything, and nothing else it might be asked to do is more urgent than acquiring contacts. But bootstrap is not only a startup step - it also fires on the periodic self-lookup and whenever the table thins below the threshold, at which point the node is serving real traffic and the priority inverts the intent, pushing user lookups behind a full fan-out of maintenance ones. The larger the routing table, the more lookups jump the queue, so the penalty grows precisely on the nodes carrying the most work. The priority now belongs to the first bootstrap only, tracked by a flag that is cleared when a bootstrap completes rather than when one is attempted. Clearing on attempt would spend the priority on a call that returned early - rate-limited, or with nothing to contact - and leave the bootstrap that actually populates the table running at normal priority, which is the one case where preempting is warranted. fillBuckets() now skips empty buckets. An empty bucket is normally an artifact of deep splitting: it covers a slice of the keyspace with no reachable nodes in it, so a lookup there converges on nothing and would be repeated at the full cost of an iterative lookup on every bootstrap for the life of the node. mldht guards the same way and for the same reason.
fillBuckets() dispatched one full iterative lookup per eligible bucket, sizing itself from the routing table while the task queue it shares stays fixed, and re-running as often as every BOOTSTRAP_MIN_INTERVAL while the table is below the bootstrap threshold. It now selects at most MAX_BUCKET_FILLS_PER_BOOTSTRAP buckets, most overdue first, skipping buckets the cheaper ping-refresh path is already repairing and those filled within the last BUCKET_REFRESH_INTERVAL. Deferred buckets stay stale and rotate in on the next bootstrap. Nothing fans out at all while the table is too thin to route, except on the first bootstrap after startup, where the table is small by definition and latency matters more. Enforcing the per-bucket limit needed a second timestamp on KBucket. The existing lastRefresh belongs to the ping path and is not only a clock - put() zeroes it to demand the eviction probe when a reachable node arrives at a full bucket - so the fill path's optimistic stamp was discarding that request and suppressing ping refresh for fifteen minutes on a lookup that may have found nothing. The two paths now keep separate clocks.
The RPC server already knew when the local socket had gone deaf, but only three call sites asked. Everything else kept running: the periodic re-bootstrap, the bucket-filling fan-out, and - much the largest - persistentAnnounce, which fires one full iterative lookup per persisted value and per peer every five minutes. While unreachable, none of it can do anything but time out. Those are now gated, and the ones deliberately left ungated say why: randomPing is how the node notices the network came back, inbound requests are proof the socket works, and anything the application asked for outranks our own guess about connectivity. The gate needed a detector that tells the truth first. checkReachability required a previously received packet before it would ever report unreachable, so a node whose network was broken from the start reported itself connected indefinitely - exactly the node whose background traffic is most futile. The condition it was reaching for is an unanswered request, so the server now stamps outgoing requests and asks whether one went out after the last packet that arrived. A node that sends nothing still reaches no verdict, which is what the original guard was protecting. One exception to the gate carries the design: a node whose table has drained cannot recover through randomPing, because there is nothing left to ping. That tier keeps talking to the configured bootstrap servers even while deaf, and the interval is what bounds it. Which raised whether that interval should back off, as the review proposed. It should not. After the fan-out cap and this gating, a failing node's whole cost to shared infrastructure is one findNode packet per configured server per interval, and backoff would do its worst damage in the case that motivates it - the whole stranded population sitting at its longest interval exactly when the servers come back. High churn also undercuts the premise, since an attempt that found nothing four minutes ago says little about a peer set that has partly turned over. What survives is the half of the argument about correlation rather than volume: a symmetric jitter band, so nodes that started together drift apart without the mean cadence moving. It has to be wider than one update tick or the tick rounding absorbs it - the reasoning is on the constant, which is now BOOTSTRAP_INTERVAL, since with a band either side of it the old name was no longer true.
An audit of what can block or delay a bootstrap. Three of the four are permanent once entered. A deaf node never asked a bootstrap server again: the tier is chosen from the routing table, and a deaf node's table neither answers nor drains, since every eviction path is gated on reachability or ignores staleness. Being unreachable now selects that tier outright, and a server's reply restores reachability on its own. fillHomeBucket is skipped when we are deaf and the servers answered nothing, so a deaf retry costs one packet per server rather than a futile lookup. The bootstrapping latch could stick set for the life of the node, because it is cleared at the end of a chain that two error paths let hang: RpcServer.stop dropped its pending calls instead of cancelling them, and TaskManager.add returned silently when it rejected a task. Both now end the work properly. Bootstrap state was initialized per object rather than per deployment, so a redeploy inherited a stale lastBootstrap and skipped its startup bootstrap. Resetting it in deploy also clears maintenanceTasks, whose stale entries would otherwise exclude a bucket from every refresh path forever.
ClosestSet.isEligible() required more consecutive non-improving responses than the closest set has slots, so raising k did two things where only one was intended: it made the routing table more robust, and it doubled what every lookup on the node costs. At k=16 convergence needed about 33 responses against k=8's 17. The counter is not the termination rule, which is the part worth stating plainly because the name suggests otherwise. LookupTask.isDone() ands it with "no unqueried candidate is closer than the closest set's tail", and that test is sound on its own - nothing left to ask can enter the set. What the counter adds is an exploration margin spent past that point, on the chance that a node farther from the target knows a closer one no response has mentioned yet. Worth keeping. It has no reason to grow with k, though, and arguably should shrink: each response carries up to MAX_NODES_PER_RESPONSE nodes, so a larger k makes a false plateau less likely, not more. The expression came from mldht, whose closest set sits at that project's own k of 8; this implementation inherited it and then raised k. Same shape as the bootstrap thresholds fixed earlier - a value calibrated at k=8 that silently doubled - which is why the margin is pinned at 8 rather than replaced with a new number. The margin is now min(k, 8), read through one ClosestSet.stabilityMargin() that LookupTask derives its iteration budget from as well, so the rule and the budget that must not fall below it cannot drift apart. A node at k=8 or below is unchanged; at k=16 a converging lookup needs about 25 responses instead of 33, roughly a quarter less traffic on every lookup the node performs. The result is the same either way - the set still returns k contacts, which is what the announce path consumes. The budget moved with it, and one term of it turned out to be backwards. The floor is now exact, k + margin + 1 rather than "about 2k", and the slack lost its max(k, ...) wrapper: that grew the slack as k grew, while the depth ramp it exists for shrinks with k, since convergence is O(log_k N). It bound only from k=25 up, so its entire effect was to inflate super-node budgets - 192 to 97 at k=64, and 56 to 49 at the default. LOOKUP_CONVERGENCE_FACTOR is retired; its whole content was the "one k to fill, one k to stabilize" derivation that no longer holds. The slack is not cut further because an iteration is not an RPC. Task.tryIterate runs on every call state change at or past STALLED, and a call stalls as soon as it outlives the timeout sampler's estimate - a percentile, so a share of healthy calls stall by construction. A fast response costs one iteration, a slow one two, a lost one two plus its retry. The budget therefore over-counts RPCs by up to a factor of two exactly when the network is bad. Measured against that, the old 56 sat 23 iterations above its floor and the new 49 sits 24 above a smaller one: the same absolute margin, and cutting it would truncate lookups on slow paths, which report COMPLETED like any other.
doBootstrap sent a findNode to every bootstrap node and waited for all of them with Future.all, so a bootstrap finished at the pace of the slowest node - and an unanswered call only reaches a final state when its RPC times out, ten seconds later. The finding filed this as load on shared infrastructure, but the cost that bites is latency, and it lands at startup: deploy() puts the bootstrap future into connectFutures and only reports ConnectionStatus.Connected once they resolve, so one dead entry in the configured list left a node calling itself disconnected for ten seconds after it was perfectly usable. The fan-out moved into askBootstrapNodes, which resolves on the first response plus a one-second grace rather than on the last. Any response arms the grace, including one from a node whose own table is empty: that answer seeds nothing, but it proves the path works, and a slower node that does carry nodes still lands inside the window - waiting for a non-empty answer instead would reintroduce the same ten seconds whenever a fresh node shares a list with a dead one. Nothing is abandoned by resolving early. The outstanding calls stay outstanding and a late response still runs the normal receive path, so its sender still enters the routing table; all that is given up is the node list it carried, by which point fillHomeBucket is already running on another node's. Undeploy mid-bootstrap is safe for the reason it was before: Vert.x drops context timers, but RpcServer.stop() cancels pending calls, CANCELED is final, and the all-settled path completes the promise. The load half is narrower than it looks, because selectBootstrapTier only reaches this path for a node that is deaf or thin-tabled - a healthy node never contacts the configured nodes at all. What is worth bounding is the mass reconnection after an outage, so the periodic attempt now draws through selectBootstrapNodes, a pure function capped at BOOTSTRAP_NODES_PER_ATTEMPT. At 8 that leaves any ordinary configuration untouched and binds only on a pathological list, where a node's load would otherwise scale with however many entries someone pasted in. Redundancy should buy resilience, not per-attempt traffic. The cap applies to the periodic path alone: startup contacts everything, because first contact is where latency matters most, and bootstrap(nodes) contacts what the application named. The draw is fresh each attempt, so no node is permanently unlucky and a recovered one is picked up again without any health tracking.
… todo initialization
…pass that reaches everything A node that restarts from a persisted routing table queued one PingRefreshTask per loaded bucket and put each one's promise into connectFutures. That gated the connection status on the slowest bucket - and it was the only thing gating it, because RpcServer.reachable starts out true and only notifies on a change, so the handler that would otherwise report Connected is silent at startup. The cost is the same ten seconds the bootstrap fan-out used to pay: a cached contact that has gone away is silent rather than refusing, so its ping settles only at RPC_CALL_TIMEOUT_MAX, and a node that has just started has no RTT samples, so TimeoutSampler hands out exactly that maximum until something answers. One dead contact among a bucket's first alpha entries was enough, which is nearly every warm start. The sweep now reports Connected the moment any cached contact answers, through a first-response hook on the task. One answer is simultaneous proof that the socket works and that the table is not a graveyard, which is the whole of the status question. It announces that directly rather than merely resolving its future, because the combinator also waits on the bootstrap, and on a stale cache the bootstrap is the slower of the two - its lookups route through the same dead contacts. Resolving the sweep alone would have changed nothing observable. Coverage and concurrency turned out to be separate questions, and only the second one starved anything. What crowds out the bootstrap is claiming every runner at once; TaskManager does not preempt, so `prior` moves a task to the head of the queue without conjuring a free runner. The sweep therefore holds at most half the slots and refills as tasks finish, working through the whole loaded table rather than the front of it. That is what lets the entire cache be purged of contacts that no longer answer - a cache cleaned at the front and dirty at the back would need a second mechanism to finish the job, and would be the worst of both. The tail is nearly free where it matters: the first response feeds the sampler and collapses the timeout, so a live cache is swept in about as long as it takes to hear back once. Only an all-dead cache pays the timeout per batch. The future settles once a batch's worth of tasks has finished, so the status decision never waits on coverage it does not need. The same fan-out existed a third time, on the path that repeats forever. RoutingTable.maintenance reported every bucket wanting a refresh and the caller turned each into a task, so deferring work at startup did not avoid a burst - it postponed one. The handler now collects and the caller decides how much to serve: a quarter of the slots, counted against tasks still running rather than against this pass, so overlapping passes cannot accumulate. maintenance() keeps its full walk, since merging, cleanup and replacement promotion are local bookkeeping that has to cover the whole table. Both selections order by XOR distance from the local id, nearest first, since those buckets hold the contacts a lookup actually routes through. Prefix depth is not a substitute: it counts a prefix's fixed bits and says nothing about whether they match ours, and needsSplit here splits any full bucket whose new entry lands in the high branch, so a far branch can be deeper than the home bucket. The maintenance pass sorts on staleness first and distance second, which is not decoration. Distance alone starves the tail outright - the nearest are served, fall due again a refresh interval later and win again, ahead of buckets never served at all - and a warm start is exactly the case with more eligible buckets than an interval has capacity for. PingRefreshTask stamps what it covers, so least-recently-refreshed first reaches the whole table before anything repeats. It is the same key, for the same reason, that already orders the bucket-fill path.
Every re-announce cycle started one iterative store-or-announce lookup per persisted item at once. That was never unbounded concurrency - TaskManager caps running tasks and queues the rest - but it was unbounded queue depth, and four tasks per item once the nested announce and both stacks are counted. The queue is FIFO and only the bootstrap adds with priority, so a user lookup arriving mid-cycle waited behind the whole backlog. Items now run a few at a time and refill as they finish, so the bound is on the queue depth that actually hurts. Applying a budget exposed that the selection was ordered backwards. Both queries sorted by announced time descending, so the head of the list was the item announced most recently - the least urgent one in it. Harmless while everything was dispatched at once, starvation as soon as a budget takes only a prefix, and unlike a deferred bucket refresh a deferred announce means the network drops the item. The retry order was inverted for the same reason: the announced time is stamped on success only, so an item whose announce failed kept its old timestamp and sorted to the back. Ascending fixes both and makes the rotation free - served items sort away, failed ones stay at the head. A cycle still draining no longer re-selects, since the announced times have not moved and it would queue duplicates of what is already running; it warns instead, which is how falling behind becomes visible. Values and peers are queued alternately so neither starves the other.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.