docs: overlay hashes vs payloads (pull by Hash) - #5443
drQedwards wants to merge 2 commits into
Conversation
Document that FloodGate is keyed by Hash, ItemFetcher anycasts GET_TX_SET / GET_SCP_QUORUMSET by hash, and pull-mode tx dissemination uses FLOOD_ADVERT / FLOOD_DEMAND so a peer that already has a hash never re-downloads the envelope. History archives stay off the overlay. No consensus or protocol change.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Documents hash-based overlay dissemination and links subsystem guidance.
Changes:
- Explains push, pull, and anycast message flows.
- Adds an overlay README pointer.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
docs/overlay.md |
Documents hash and payload handling. |
src/overlay/README.md |
Summarizes and links overlay documentation. |
Suppressed comments (2)
docs/overlay.md:38
GET_SCP_STATEis not an anycast-by-hash operation handled byItemFetcher: it carries a ledger sequence (Peer::sendGetScpState) and directly asks that peer to send SCP envelopes. Only transaction-set and quorum-set fetches use the twoItemFetcherinstances.
- **Anycast by hash:** `GET_TX_SET` / `TX_SET`, `GET_SCP_QUORUMSET` /
`SCP_QUORUMSET`, `GET_SCP_STATE`. `ItemFetcher` asks connected peers, in
sequence, for the body of a hash. These messages are not flooded.
docs/overlay.md:50
- These lines conflate two distinct hashes.
Floodgate::broadcastindexesmFloodMapwithxdrBlake2(*msg), while the optional argument is the transaction-envelope hash passed tosendAdvert; it is not the key used to decide whether theFloodgatebroadcast record is new.
So a peer that already holds a given hash never downloads the envelope again.
That is the same rule as `broadcastMessage(..., std::optional<Hash>)`: when a
transaction is flooded, its envelope hash is what overlay uses to decide
whether the message is new.
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
docs/overlay.md:28
- This opening incorrectly generalizes transaction pull mode to the whole overlay:
SCP_MESSAGEand survey traffic are sent as full pushed/broadcast messages. Narrow the statement to the hash-pulled object types; the replacement also uses the actual class name,Floodgate.
This issue also appears in the following locations of the same file:
- line 30
- line 36
- line 47
docs/overlay.md:31
- This sounds exhaustive, but the union has additional message families such as authentication, flow-control, advert/demand, and survey messages. Present these three groups as representative rather than claiming that all
StellarMessagevariants split this way.
`StellarMessage` (see `Stellar-overlay.x`) splits into three kinds, documented
in `src/overlay/OverlayManager.h`:
docs/overlay.md:38
GET_SCP_STATEis neither keyed by a hash nor driven byItemFetcher: it carries a ledger sequence and directly causes the selected peer to send stored SCP envelopes. Also, current tx-set replies can beGENERALIZED_TX_SET, so the documented request/reply pair is incomplete.
- **Anycast by hash:** `GET_TX_SET` / `TX_SET`, `GET_SCP_QUORUMSET` /
`SCP_QUORUMSET`, `GET_SCP_STATE`. `ItemFetcher` asks connected peers, in
sequence, for the body of a hash. These messages are not flooded.
docs/overlay.md:50
- The optional transaction hash is not the key
Floodgateuses to decide whether a message is new.Floodgate::broadcastalways keys its record withxdrBlake2(*msg); the optional full transaction hash is instead supplied tosendAdvert, so these are separate hash domains.
So a peer that already holds a given hash never downloads the envelope again.
That is the same rule as `broadcastMessage(..., std::optional<Hash>)`: when a
transaction is flooded, its envelope hash is what overlay uses to decide
whether the message is new.
drQedwards
left a comment
There was a problem hiding this comment.
Do these functions need to be C coded into a file or a new file made according to this?
|
It's because I did that separate joke PR wasn't it? All well, you're absolutely right, (Says ChatGPT me), those contracts aren't fully a Soroban smart contract address as primitives for the interchainer tool |
Summary
docs/overlay.mdalready says block/tx flooding is pull-based and SCP is push-based. This PR names the actual machinery so a first-time reader of overlay does not have to reverse-engineerOverlayManager.handStellar-overlay.x.No protocol or consensus change.
What
Document that:
FloodGateis keyed byHash.GET_TX_SET/GET_SCP_QUORUMSETviaItemFetcher).FLOOD_ADVERT) and demands missing ones (FLOOD_DEMAND); the envelope is sent only if the peer does not already have that hash.docs/architecture.md).A short pointer is added in
src/overlay/README.md.Why
Peers on this network already treat a 32-byte hash as the thing that is flooded, and the payload as something you pull iff you lack it. That is easy to miss from the current overlay glossary. Naming
FLOOD_ADVERT/FLOOD_DEMANDandItemFetchermatches the headers and XDR.Test plan