Return PrunedError when trying to read unavailable historical data #13014
Conversation
erigon-lib/state/aggregator.go
Outdated
| @@ -1727,6 +1727,18 @@ func (a *Aggregator) BuildFilesInBackground(txNum uint64) chan struct{} { | |||
| return fin | |||
| } | |||
|
|
|||
| func (ac *AggregatorRoTx) StartingTxNum() uint64 { | |||
There was a problem hiding this comment.
I think we must not think that all domains are equal:
- commitment domain already has disabled history
- we may have new domains in future
- Historical exec does read: acc, storage, code, receipt. but it doesn’t read commitment domain.
so, maybe we must accept list of domain names as a parameter.
There was a problem hiding this comment.
The thing is when we create the historical state reader using rpchelper.CreateHistoryStateReader we don't pass the domains we need for it, just the txNum.
If I make the start tx num calculation function take a list of domain names as a parameter, that means that list needs to be specified by the caller of rpchelper.CreateHistoryStateReader which is not very optimal I think.
What I notice is that HistoryReaderV3 is mainly used as a StateReader, and seems to only read from:
- accounts
- storage
- code
Receipts and commitment history don't seem to be accessed.
Do you think it would make sense to consider accounts, storage, and code history in unison (as a single reader), and have separate readers for receipts and commitment? (this I can ignore receipts and commitment history startingTxNum for this reader)
There was a problem hiding this comment.
HistoryStateReader - is the "buisness-logic thing" - means only HistoryStateReader knows which domains it using. HistoryStateReader - has only 1 use-case: execution of historical blocks (other places in code can call GetAsOf method directly). And re-execution require only acc/code/storage domains.
which is not very optimal I think - what do you mean?
Don't forget that Commitment history is disabled - even on archive mode.
and have separate readers for receipts and commitment? - we already have: ReceiptGenerator. If some code need commitment - it can call Get or GetAsOf directly - don't need reader (or need some very-specialized reader).
Any idea on: what to do in --prune.mode=minimal? Here can have 0 history files but some history in DB (1 step).
There was a problem hiding this comment.
// return the earliest known txnum in state history (excluding commitment and receipt history) - Commitment history is disabled (no history files. no history data in db).
There was a problem hiding this comment.
Precisely because we don't make use of Commitment history or Receipts history, I am grouping Accounts, Storage and Code together because they are intertwined together. We can add additional methods in the future to get the start of Commitment and Receipts history.
antonis19
force-pushed
the
check-txnum-state-reader
branch
from
eb7b973 to
bf6f02b
Compare
| @@ -464,6 +464,9 @@ type TemporalTx interface { | |||
| Tx | |||
| TemporalGetter | |||
|
|
|||
| // return the earliest known txnum in state history (excluding commitment and receipt history) | |||
| StateHistoryStartFrom() uint64 | |||
There was a problem hiding this comment.
Tx/TemporalTx interface - is low-level interface. it must be business-logic agnostic.
"State" - it's something from Ethereum business-dictionary.
TemporalTx must not know - which domains are exist (or may be created in future). It also doesn't know what stored inside each domain (for TemporalTx - it's just some keys/values/timestamps).
There was a problem hiding this comment.
Fair point, I'll check if I can make it more general, i.e. HistoryStartFrom()
Certain RPC queries require access to historical data, however this data may have been deleted in a full node due to pruning. To avoid unexpected errors down the line it would be better to throw a more indicative error earlier in the request handling flows when the
HistoryReaderV3is created.Issue: #12977