Account_history plugin: notify all related accounts after a new account is created

[abitmore]

It's ideal to notify the new account, the registrar, the referrer and the ones in the owner authorities and the active authorities. Now only the new account and the registrar get notified.

Related code is here:

      if( op.op.which() == operation::tag< account_create_operation >::value )
         impacted.insert( oho.result.get<object_id_type>() );
      else
         graphene::app::operation_get_impacted_accounts( op.op, impacted );

It should be:

      if( op.op.which() == operation::tag< account_create_operation >::value )
         impacted.insert( oho.result.get<object_id_type>() );
      graphene::app::operation_get_impacted_accounts( op.op, impacted );

However, if we simply fix it this way, after a replay, there will be more account_transaction_history_object objects inserted into the object database, so object IDs of individual records will be changed, which will cause trouble for 3rd applications that rely on the consistence of the IDs.

Please discuss.

Update: This impacts history_api::get_relative_account_history API which relies on account_transaction_history_object::sequence and account_statistics_object::total_ops.

Update: PR #406 is trying to add an option to the plugin that provides the missed data when enabled, but disabled by default.

Update: fixed by PR #2480 - after the hard fork time (HARDFORK_CORE_265_TIME), new account creation operations will notify all impacted accounts (note: voting_account is still not in the impacted accounts list).

[oxarbitrage]

i was watching this issue since a few days and i see your point, however i don't have an actual solution by now to the problem. i want to add @elmato here he maybe haves an idea.

[elmato]

@abit It can cause problems if the 3rd party stores the ids in database and use them as identifier for the operation itself. (use the 1.11.X id to identify each operation, instead of (block,trx_in_block,op_in_trx) tuple) The only one i can think of is cryptofresh. Maybe we can ask them? The code for this to work is already in impacted.cpp. ... void operator()( const account_create_operation& op ) { _impacted.insert( op.registrar ); _impacted.insert( op.referrer ); add_authority_accounts( _impacted, op.owner ); add_authority_accounts( _impacted, op.active ); } ...

…

On Fri, May 5, 2017 at 7:42 PM, oxarbitrage ***@***.***> wrote: i was watching this issue since a few days and i see your point, however i don't have an actual solution by now to the problem. i want to add @elmato <https://github.com/elmato> here he maybe haves an idea. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#265 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOUohcAC4zdtoNKC01IHK-52zREubv6ks5r26XCgaJpZM4NEMWr> .

[elmato]

Now that i'm looking at the code:

The id of the account_transaction_history_object is never returned (2.9.X), instead we always return the operation_history_object id (1.11.X).
So operation_history_object ids will be the same even if we apply your patch and replay the blockchain.

[abitmore]

Afaik bittrex is storing object ID's (1.11.x) for withdrawals (edit: and deposits). It could be OK if the patch doesn't affect this ID.

[abitmore]

It's possible that account_transaction_history_object ID's are being used by 3rd apps to track deposits, for example, to store the latest processed deposit.

[elmato]

Just to make it clear, because my previous comment was misleading.

1.11.X objects are part of the state, so they preserve the I'd no matter what we do in any plugin.

2.9.X objects are implementation details and they can change if we modify the plugin as you posted in the first comment.

The RPC API always return 1.11.X objects so we can rest assured that 3rd parties can rely on this id.

2.9.X objects are never returned by the RPC api

[abitmore]

OK, I got it.

The 2.9.X ID's are not being returned by get_account_history API, only 1.11.X ID's are being returned.

However, the get_account_history API was buggy (for example #92, #168, #193), so some applications were using get_object API with 2.9.X ID's to fetch account histories by their own. That's why I thought it's a bit risky to change the code.

[oxarbitrage]

Hi guys, 2.9.X just for your consideration objects are yes returned by the api already.

here is a sample, request an account will return between other things a statistics object.

root@alfredo:~# curl --data '{"jsonrpc": "2.0", "params": ["database", "get_objects", [["1.2.167006"]]], "method": "call", "id": 10}' http://127.0.0.18090/rpc | jq

...
"statistics": "2.6.167006",
...

Inside this statistic object most_recent_op is a 2.9.X object.

root@alfredo:~# curl --data '{"jsonrpc": "2.0", "params": ["database", "get_objects", [["2.6.167006"]]], "method": "call", "id": 10}' http://localhost:8090/rp
c | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   305  100   202  100   103   288k   147k --:--:-- --:--:-- --:--:--  197k
{
  "id": 10,
  "result": [
    {
      "id": "2.6.167006",
      "owner": "1.2.167006",
      "most_recent_op": "2.9.15497698",
      "total_ops": 24,
      "total_core_in_orders": 0,
      "lifetime_fees_paid": 807192,
      "pending_fees": 0,
      "pending_vested_fees": 0
    }
  ]
}
root@alfredo:~# 

By now, i think it is exposed just as last op. The client may be relying on the object id itself or not but it is possible for some that a change here can break their programs.

[abitmore]

This impacts history_api::get_relative_account_history API which relies on account_transaction_history_object::sequence and account_statistics_object::total_ops.

[abitmore]

#276 is related.

[abitmore]

I think we can add an option to the plugin that provides the missed data when enabled, but disabled by default.

[oxarbitrage]

i am going to work on this as an additional plugin option disabled by default as i am already making some work in the account_history_plugin and this should not be very hard to add.

[oxarbitrage]

#406

[abitmore]

Lately I think it's better to fix the issue in a consensus-upgrade style, although strictly it's not a consensus change, because many 3rd-party applications rely on consistent numbering in account history. That said, only track all related accounts (for newly created accounts) after a defined time.

[pmconrad]

The primary purpose of account_transaction_history is to provide an auditable log of balance changes. I think we shouldn't abuse it as some form of generic notification infrastructure, especially because of the numbering issue.
Better provide a separate mechanism for historic events.

[abitmore]

On the contrary I don't think it's abuse, but is fair use. I agree with a separate mechanism though. For example, classify the events in account history.

[abitmore]

Fixed by #2480.