Skip to content

Conversation

codablock
Copy link

This indicates a previous crash where the TX made it into the txindex but
the block was not flushed to disk. When dashd is restarted then, there is
a short time where GetTransaction would return a non-existant block, while
callers very often assume that the returned block hash is known.

This indicates a previous crash where the TX made it into the txindex but
the block was not flushed to disk. When dashd is restarted then, there is
a short time where GetTransaction would return a non-existant block, while
callers very often assume that the returned block hash is known.
@codablock codablock added this to the 16 milestone Jul 13, 2020
Copy link

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, but just as im interested, do you have more details to share about where this issue/crash happens?

@codablock
Copy link
Author

@xdustinface I obseved this while someone from the community was doing stress tests on testnet. Many of my nodes crashed due to OOM (I didn't reserve enough RAM per tMN) and then failed to start again, as they immediately received IS locks from other nodes which made them crash in IS code (here). Instead, the IS locks should have been handled as if the TX was unknown locally.

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@xdustinface
Copy link

xdustinface commented Jul 13, 2020

@xdustinface I obseved this while someone from the community was doing stress tests on testnet. Many of my nodes crashed due to OOM (I didn't reserve enough RAM per tMN) and then failed to start again, as they immediately received IS locks from other nodes which made them crash in IS code (here). Instead, the IS locks should have been handled as if the TX was unknown locally.

@codablock thanks! So if i got it right it this means we have inconsistencies between the block data and the txindex if this happens? Is this at least resolved somehow later(probably when the block gets added properly again)? :D Also, could we prevent this by moving the txindex batch write from CBlockTreeDB::WriteTxIndex to FlushStateToDisk (for sure with the other adjustments to make that work)?

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@codablock
Copy link
Author

@xdustinface Yes we have inconsistencies in the case of a crash, but we can ignore them (with this patch applied) as the only place where the index is read is in GetTransaction.

@xdustinface
Copy link

@codablock alright. Means we should keep an eye on the future usage of ReadTxIndex hehe. Anyway, i guess this is ready to be merged :D

@codablock codablock merged commit 1cb3f90 into dashpay:develop Jul 14, 2020
@codablock codablock deleted the pr_fix_gettransaction branch July 14, 2020 09:22
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Aug 23, 2020
…ex (dashpay#3606)

This indicates a previous crash where the TX made it into the txindex but
the block was not flushed to disk. When dashd is restarted then, there is
a short time where GetTransaction would return a non-existant block, while
callers very often assume that the returned block hash is known.
@PastaPastaPasta
Copy link
Member

backported in #3670

gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 28, 2022
…ex (dashpay#3606)

This indicates a previous crash where the TX made it into the txindex but
the block was not flushed to disk. When dashd is restarted then, there is
a short time where GetTransaction would return a non-existant block, while
callers very often assume that the returned block hash is known.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants