-
Notifications
You must be signed in to change notification settings - Fork 9
Conversation
@nionis I added a commit; let me know if you agree with all of the changes there. a manual test on my laptop is working afaik (need to test data consistency, though). I think the bug when ctrl-c has been fixed, somehow. But I'm doing some testing work as my next focus, which will definitely be testing this behavior. |
@@ -64,34 +63,46 @@ class ReducerRunner { | |||
// we're resuming, so replay from store if possible | |||
try { | |||
const latestTransaction = await globalState.store.getLatestTransaction(this.reducer.config.key) |
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.
does getLatestTransaction include historicalblocks transactions of the reducer?
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.
it doesn't; this gets the latest Ourbit transaction (which isn't a blockchain transaction, but I didn't have a better name for it :( )
} | ||
|
||
try { | ||
const mostRecentHistoricalBlock = historicalBlocks[historicalBlocks.length - 1] | ||
|
||
assert.equal( |
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.
lets say last transaction was at block 100
and we let the reducer run until the saved chain has blocks from 101 - 201
this is only okay if getLatestTransaction includes historicalblocks transactions, if it does not include historicalblocks transactions then there is a possibility that the last reducer transaction was done 100 blocks ago
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.
lastTransaction should always be the most recent Ourbit#Transaction
, which is produced after a block is found, just like historical blocks. The only time this won't be true is if the process crashing right in the middle of the part where we commitTransaction
and saveHistoricalBlock
, right? Maybe I'm misunderstanding, sorry
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.
so if I understand correctly, Ourbit#Transaction will be produced every block since we save the historical block into the db?
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.
An Ourbit#Transaction
is the set of all of the patches and operations that were performed in one atomic set (like a blockstream block). So it is produced for every block (after that block has been processed), but not because of the historical blocks; it's job is to track the patches so that we can potentially revert them later.
@shrugs The state issue is not fixed, I still get it on my end when I ctrl-c and restart a couple of times, it happens when applying patches
The issue turns out to be coming from erc20 reducer, and maybe its the use of 2 operations, I am moving this to a new issue #48 |
I think it's still possible to move this historical block code into the blockstream and take it out of the reducer runner; that way it's the blockstream doing all of the work on itself. It just has to know about the store. Do you think that's a better structure? |
@shrugs up to you, I don't really know |
@nionis I'll try it out on a commit and you can tell me if it looks ok. hopefully I finish before my laptop runs out of battery right now 😂 |
@nionis let me know how you feel about that last commit; not super tested because my laptop is dying, but feel free to change or revert it |
Pull Request Test Coverage Report for Build 30
💛 - Coveralls |
I'm working on https://github.com/XLNT/gnarly/tree/feat/block-retention for now, with some refactor/test commits. Don't want to push them here because they're incomplete, but check them out before you build on top of this branch at the moment. |
added a bunch of blockstreamer tests, so that's looking pretty good. any extra cases we should test? I'm sure there are more |
merged 🎉 |
will flesh out more tests in the coming days, but I'm happy with the suite enough to merge this bigboi pr |
this commit is unfinished / buggy work on block-retention
todo: