steemit / steem-python

The official Python (3) library for the Steem Blockchain.
https://steem.io
MIT License
154 stars 100 forks source link

Adds support for Appbase #192

Closed cyon1c closed 6 years ago

cyon1c commented 6 years ago

Branch of #166.

On node initialization, http_client will attempt to request the node_version from condenser_api.get_version.

All tests pass except account_history.

roadscape commented 6 years ago

It seems the account history test might be failing partially because of a bug in steemd's ah pruning where the very first item in history is always present. The test is not very strict though, so it might actually pass if there wasn't some quirk in history_reverse which causes op 0 to be saved multiple times:

[0, 3909, 3910, 3911, 3912, 3913, 3914, 3915, 3916, 3917, 3918, 3919, 3920, 3921, 3922, 3923, 3924, 3925, 3926, 3927, 3928, 3929, 3930, 3931, 3932, 3933, 3934, 3935, 3936, 3937, 3938, 3939]
[0, 0, 0, 3909, 3910, 3911, 3912, 3913, 3914, 3915, 3916, 3917, 3918, 3919, 3920, 3921, 3922, 3923, 3924, 3925, 3926, 3927, 3928, 3929, 3930, 3931, 3932, 3933, 3934, 3935, 3936, 3937, 3938, 3939]
roadscape commented 6 years ago

next step is to audit the appbase-switch logic, and run tests against:

use dev environment for both as it has all the steemd fixes for passing tests.

the main problem here is recursive loop because set_node now makes an API call -- which, upon failure, will again call set_node.

roadscape commented 6 years ago

Also, we are not saving the appbase flag per endpoint, and we make a call every single time we switch to a new node (and I guess after each RPCError) -- resulting in extra complexity in the request failure flow (which is already too much).

How about this approach:

  1. assume appbase/condenser_api by default
  2. when we encounter an RPCError:
    • if error message signals the endpoint is pre-appbase,
      • add the current endpoint to a runtime downgrade list
      • if already in downgrade, raise exception (apparently, signal unreliable)
    • retry as usual (and assume appbase/condenser_api unless node is in downgrade)

If we can find a reliable error string from pre-appbase nodes this approach will be much cleaner.

sneak commented 6 years ago

Maintaining BC is not important for very long, as with HF20 everything will be appbase.

cyon1c commented 6 years ago

@sneak we have to maintain BC for the time being for devs hitting any non-appbase node.

sneak commented 6 years ago

Of course - I'm just saying that we probably shouldn't invest significant engineering into architecting a toggle that's going to be ripped out permanently in a few months. I think it's as simple as branching on the presence of a STEEM_DONT_USE_APPBASE env var, and defaulting to appbase.

sneak commented 6 years ago

We certainly don't need to branch on a per-server level. It can be processwide, and it can simply default to appbase.

roadscape commented 6 years ago

Alternative implementation at #202