-
Notifications
You must be signed in to change notification settings - Fork 648
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
Add extended history tracking for select accounts #2259
Add extended history tracking for select accounts #2259
Conversation
Note: code compiles and starts up, but I haven't done a full replay yet to confirm that it is indeed tracking extended history for my accounts. So this code is still in "testing". @abitmore — I assume this will need to be rebased onto another branch — let me know if develop, hardfork, or elsewhere. |
Please rebase onto |
1e5a7a6
to
0fe9af9
Compare
@abitmore wrote:
Done. Also my node finished replay and I confirmed it is tracking extended history for the named accounts/registrars and regular (short) history for all other accounts. Looks to be working. (Although my hopes for extended history in the Reference GUI did not pan out — but this seems to be a limitation of the UI, not the API node. Uptick was able to pull all the extended history.) |
Thanks. I'll review soon. |
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.
Thanks for the efforts.
The implementation would work for typical use cases. However,
- it does not work if
_max_ops_per_account
is set to0
butextended_*
is set, especially if the node operator mainly wants to track by registrars; - it does not work if
_tracked_account
is not empty andextended_*
is not within_tracked_accounts
; - performance can be improved (IMHO) quite a bit as mentioned in the in-line comments;
- tracking by registrar stops when the account upgrades to LTM (The
extended-history-by-registrar
option in account history plugin does not track LTM correctly #2656).
extended_hist |= (account_id == eh_account_id); | ||
} | ||
if ( _extended_history_registrars.size() > 0 ) { | ||
const account_id_type registrar_id = account_id(db).registrar; |
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.
Querying DB once on every operation is a bit inefficient. Perhaps better create a new index in the plugin to save the tracked accounts. Why a new index? Because the list is dynamic and grows over time.
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.
Although the account_index
is big, the complexity of querying it by ID is linear, so the performance hit is probably not significant. Best if we can have some data to show the differences E.G. time spent for replaying with the old code v.s. the new code.
I will decide later whether to merge it as is. |
What about something more flexible and powerful like this?
may specify multiple times
or as a list of lists
You have specific |
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.
Approving and merging so far. We can improve later.
Addresses #2258 - "Extended operation history for accounts selected by ID or by registrar".
Adds the following options to
account_history
plugin:This PR may also add a partial, if imperfect, solution to #2117 "Track accounts using wildcards".
Note by @abitmore: check this comment for limitation of current implementation: #2259 (review)