-
Notifications
You must be signed in to change notification settings - Fork 471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix missing tx index on reorg #8027
Conversation
byte[] txIndexData = _transactionDb.Get(tx.Hash); | ||
if (txIndexData is not null && txIndexData.Length != Hash256.Size) | ||
{ | ||
long blockNumber = new RlpStream(txIndexData).DecodeLong(); | ||
if (blockNumber != block.Number) continue; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe simpler fix would be never remove block transactions and allow for it pointing to a block where it doesn't exist, and then handle that on searching for receipt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I guess that works too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are still removing it though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method also use to remove old tx index (the tx lookup limit config). So its no longer called during reorg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah you are right, I was confused about method being still there, but it is for pruning.
Can you confirm it doesn't break eth_getLogs
and eth_getTransactionReceipt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getlogs and gettransactionbyhash still works
Fix #6560
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing