Repository navigation
Trinutary SDK Cleanup and Testing - #1276
Open
kayabaNerve wants to merge 36 commits into
Open
kayabaNerve wants to merge 36 commits into
kayabaNerve wants to merge 36 commits into
Conversation
While this keeps the overall structure and ideology, this redoes the internal implementation for the necessary code quality and security reasons. This has impact for #806 due to how this is now a robust, well-documented library where Serai-specific (or Serai-inspired) constants have been removed in favor of a more modular design. This also has impact for #646 due to reworking how the items within blocks can be synced and gossiped, by a capable P2P layer. Fixes #1237. #1236 was effectively fixed by: 1) The `tendermint-machine` rewrite ensuring `Slash` was of bounded size 2) This PR removing the Tendermint transaction type, instead including slashes directly in blocks 3) Unsigned transactions having their size checked as part of its syntax, independent of its kind
This is just adding an asynchronous path from the `Tributary`. The synchronous path for `Tendermint` is left as-is, as that expects no delays to effect a slash (which usage of a DB channel solves). I don't love the current `impl Future for Mempool`. That should be improved in the future.
The biggest change to `tendermint-machine` is the introduction of `Block::proposal_hash` and `Block::semantic_hash`, replacing the single `Block::hash` method. This is used to ensure a proposed block can't be malleated upon relay, yet the commit still binds to the semantics, allowing pruning/replacing signature data. The `NRoundMessagesToRoundMessages` iterator yielded `None` for any missing holes instead of skipping past them, now fixed by using `find_map` instead of `and_then`. `SlashReason::verify` now checks it's a validator being slashed in the `Equivocation` case. Edge cases around historic/future messages within the Tendermint consensus process are now handled. The rebroadcast timeout is now dependent on actually updating the step, not just receiving _any_ message. The `pin!(receiver.recv())` kludge, which didn't properly make use of wakers, has been replaced with use of the stream API and `poll_next`. This was a known concern but wasn't prioritized prior. Now that I'm being nagged about it, with a reasonable counterexample given, the fix is here. The `Mempool` didn't set `LastBlock` and `Blockchain` conflated the `LastSigned` nonce with the expected nonce. The former broke the mempool on reboot. The latter broke signed transactions with a nonce greater than `1`. `tendermint-machine` is documented to potentially call `Blockchain::add_block` multiple times, but this wasn't reflected in the `tributary-sdk`, potentially stalling the chain after an unfortunately timed reboot. The cache for a block's proposer has had its indexing fixed. The documentation around `Unsigned` transactions has been clarified. It's meant to be a privileged transaction, hence why it's independent of any individual validator and not subject to any rate limits. Various other documentation improvements have occurred as well. Instead of popping items one at a time, checking the block's validity after each and every pop, all items are popped at once (if possible). More intelligent and efficient methodology for popping transactions occur when creating a block proposal now, preventing a single validator from continually exhausting the block's space. I don't love how many allocations are now used (due to placing each validator's transactions in their own collection), but the algorithms themselves should be reasonable. Additionally, the mempool now applies the block's size limit to each individual validator's transactions to limit how much work is done when creating the block proposal.
This allows us to expand it to a 32-byte value, effectively ensuring it won't collide with a block's hash, which would break assumptions present in the Tributary.
This ensures an honest proposal never proposes an invalid block.
The prior `tendermint-machine` did something similar. Basically, due to the fact we could have uncommitted data in the block, there could be an invalid block with the same semantic hash as a valid block. If the local view had the invalid block, and the commit for the semantic hash, the local process would attempt to add the invalid block, causing a stall. Instead, the valid block should've been synced via the external sync loop, as if the local proposal (fatally invalid) didn't exist. Alternatively, upon receiving a proposal message for a block with the same semantic hash, yet one which isn't so invalid, that could be used in place. To fix this, fatal proposals aren't accumulated into the metrics, causing `observed_proposal` to return `None`. However, they're still accumulated into the message log, allowing us to detect if we should jump ahead/equivocations/prevent ourselves from being spammed with invalid proposals (if we didn't save them at all, we wouldn't know we've already rejected such a proposal). Unfortunately, we now need block validity in a few places: - Before accumulating into the metrics (new) - For our own proposal (thanks to a recent commit) - When deciding to prevote or not To prevent redundant verification, a block validity cache has been introduced. For each proposer, for the last block they proposed, we cache if the block was valid. If a cached result is that the block was valid, or fatally invalid, the cached result is used. This allows optimistically reducing to a single block verification, as optimal. This cache can be improved in the future by not caching per validator, per block, but solely per block (so when two proposers propose the same block, the same cache entry is used for both).
If the process rebooted after adding a block to the blockchain, but before the Tendermint process saved its decision (as they use two distinct database transactions), the local process could become corrupt. This could be recovered from via syncing the problematic block, but this is straightforward enough to resolve directly. Reported by @SamsungGalaxyPlayer.
The block proposal code did only yield proposals which satisfied the minimum time. However, it'd yield them immediately, even if they'd then be rejected as being in the future. The new logic awaits a `Sleep` future until the minimum time is satisfied, if one is present, before yielding a block with a time at least the minimum time. This causes a `BlockProposal`, ideally, to only yield proposals with valid minimum times _and_ ones which are temporally valid when yielded.
--- This does replace the prior "heartbeat"-premised syncing mechanisms with usage of the consensus process to learn if we're behind, and if so, by how much. Once we know how far behind we are, we sync all blocks/commits incrementally from there however from a small subset of peers in parallel (redundantly). In the future, it'd be better to: - Sync the block headers - Sync _all_ missing blocks in parallel in small batches - Only redundantly request blocks when there's only a few blocks still missing The current redundant sync is so a slow peer doesn't limit us to their speed. The above design allows peers to be slow, without wasting any effort, until there's no other work remaining than the work currently assigned to the slow peers. Only then when fast peers be redundantly assigned to pick up the slack. For more discussion of this, please see #646. --- `serai-coordinator-substrate` once again populates the Serai block time into the `NewSetInformation`. This was removed when `tendermint-machine` was fixed to no longer require/expect a strict clock, the only reason it was populated for. Now, it's used for the spawned Tributary's `genesis_time`, preventing a malicious supermajority from spamming backdated blocks. --- A Tributary's scanner fatally slashing a validator provokes a censure, causing the validator to no longer handle transactions from the slashed validator in the mempool. This resolves a longstanding TODO.
It's a cleaner name, one surprisingly available.
It was viable to make this change now. "SDK" may come with connotations of SaaS/this being a product of some sort, and wasn't preferable accordingly.
It implemented a yield after adding a block but failed to schedule a wake condition. It now immediately wakes itself.
This mainly smashes out some larger sections into more modular code for testing.
This prior commented certain slash handling and that's still deferred for right right now...
The intent is for the median to be considered unhealthy if either the amount of
samples isn't large enough or there's historical values present in the median
(as documented). However, the usage of `if !(a || b) { unhealthy }` meant the
median was only labelled unhealthy if both `a` and `b` (the conditions to be
considered healthy) were `false`, instead of if either condition was `false`.
The new check is rewritten for greater clarity.
This was submitted by @inc0gnito-ape as a security issue via GitHub. At this
time, it hasn't been acknowledged as a security issue. While it does defeat the
defense-in-depth measure of checking the median's health, there was a lack of
discussion on how to create the unhealthy median in the first place, and a lack
of convincing impact given the following mitigating factors:
- While this median can be used for swapping to staked SRI, staked SRI is
encumbered. A compromised set can be halted by the other validators.
- While the staked SRI could be used for key shares, prior to economic
security, the genesis validators are designed to maintain 34% of the key
shares.
- Even if this was exploited right before a set rotation occurred, the last set
(which runs for a week) would presumably have been honest and populated a
healthy median.
Within Serai's current BBP, there's also a lack of impact assigned to such
economic attacks, and I'm unsure how to rate the severity of something so
theoretical. While obviously, there is a long history of
'oracle price manipulation attacks' against many protocols, Serai's design and
considerations is incredibly different.
A good-will reward was offered to the whitehat for their report.
This is due to `crypto-bigint` yanking all `0.7` releases prior to `0.7.9`, which raised the MSRV to `1.87`.
This was referenced Nov 3, 2026
This also explicitly pins `crypto-bigint 0.7` -> `crypto-bigint 0.7.9` for clarity as `crypto-bigint >= 0.7.0, <= 0.7.8` were yanked.
A manual `a < b` sorted check was replaced by `is_sorted()`, even though `is_sorted()` uses `a <= b`. This was correctly distinguished elsewhere in the codebase but regressed here. A regression test is now included.
This branch has not been deployed
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
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.