Skip to content
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

Implement voting right delegation for oracle #148

Merged
merged 4 commits into from
May 28, 2019
Merged

Implement voting right delegation for oracle #148

merged 4 commits into from
May 28, 2019

Conversation

hendrikhofstadt
Copy link
Contributor

I implemented delegation of oracle voting rights to other Account addresses.

By using the MsgDelegateFeederPermission a validator can assign the right to vote to another account at any time. The validator account will preserve its right to vote at any time.

A single account can have the right to vote on behalf of multiple validators.

A vote message must now contain the validator to vote on behalf of and the sender/feeder address.

@dokwon dokwon requested review from dokwon and yun-yeo May 22, 2019 13:58
@dokwon dokwon added enhancement New feature or request good first issue Good for newcomers must Mustfix for target release. labels May 22, 2019
@joe-bowman
Copy link

Code looks great.

My concern here is this:

A single account can have the right to vote on behalf of multiple validators.

Given that the point of the feed is to ensure we have a stream of good data coming in from multiple distinct sources, having multiple validators being voted on by a single account somewhat subverts this. In my opinion, each validator should have its own source, else we compromise the safety of the oracle.

I feel there should be a 1:1 relationship between delegated address and operator.

Copy link
Contributor

@yun-yeo yun-yeo left a comment

Choose a reason for hiding this comment

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

Thanks for your hard works. Good improvement.

x/oracle/handler.go Outdated Show resolved Hide resolved
x/oracle/handler.go Outdated Show resolved Hide resolved
x/oracle/client/cli/query.go Outdated Show resolved Hide resolved
x/oracle/client/cli/query.go Outdated Show resolved Hide resolved
@dokwon
Copy link
Contributor

dokwon commented May 23, 2019

@joe-bowman while i agree with the motivation of wanting to variegate data sources as much as possible, the reality is there aren't that many reliable APIs that pull in crypto prices (correct me if I am wrong) today - so we may end up with different implementations that do the same thing. Moreover, we may end up with interesting secondary business models where the data source provider charges for traffic from the validator.

@dokwon
Copy link
Contributor

dokwon commented May 23, 2019

Given how columbus is pretty centralized at the moment i really hope lots of validators don't subscribe to the terraform labs' voter, however...

@joe-bowman
Copy link

joe-bowman commented May 23, 2019

@joe-bowman while i agree with the motivation of wanting to variegate data sources as much as possible, the reality is there aren't that many reliable APIs that pull in crypto prices (correct me if I am wrong) today - so we may end up with different implementations that do the same thing. Moreover, we may end up with interesting secondary business models where the data source provider charges for traffic from the validator.

EDIT: Ignore this, it's nonsense, we want to Luna price for each :)

Surely it is fiat prices we are concerned about, not crypto prices?
USD <-> KRW <-> EUR <-> CHY <-> SDR

There are plenty of resources to allow one to do this, paid for and free.

If a validator is not prepared to provide a distinct data stream service, they shouldn't reap the rewards associated with the oracle.

If a majority of validators 'follow' a single service (which actually is not how this implementation appears to currently work; the 'centralised' - for lack of a better term - service would need to submit a vote for each operator) then the 'mean' vote will always be centered around this service, thus disincentivising validators from pursuing a distinct service for fear they might lose out on fees. We then end up with everyone relying on this single service, which is of course in the interest of individual validators, in terms of rewards, but very much not in the best interest of the network as a whole.

IMO, this change as is encourages centralisation of the pricing oracle service.

@hendrikhofstadt
Copy link
Contributor Author

@joe-bowman while i agree with the motivation of wanting to variegate data sources as much as possible, the reality is there aren't that many reliable APIs that pull in crypto prices (correct me if I am wrong) today - so we may end up with different implementations that do the same thing. Moreover, we may end up with interesting secondary business models where the data source provider charges for traffic from the validator.

Surely it is fiat prices we are concerned about, not crypto prices?
USD <-> KRW <-> EUR <-> CHY <-> SDR

There are plenty of resources to allow one to do this, paid for and free.

If a validator is not prepared to provide a distinct data stream service, they shouldn't reap the rewards associated with the oracle.

If a majority of validators 'follow' a single service (which actually is not how this implementation appears to currently work; the 'centralised' - for lack of a better term - service would need to submit a vote for each operator) then the 'mean' vote will always be centered around this service, thus disincentivising validators from pursuing a distinct service for fear they might lose out on fees. We then end up with everyone relying on this single service, which is of course in the interest of individual validators, in terms of rewards, but very much not in the best interest of the network as a whole.

IMO, this change as is encourages centralisation of the pricing oracle service.

I intentionally added an iterator function to check whether some address already has a feeder right delegation so we could make this a 1:1 relationship with just a few LOC.

However, the way I implemented this does not mean that someone could simply "subscribe" to another validator's datastream. The feeder/delegate has to intentionally send oracle votes for a specific validator.

So if everyone would want to use the Chorus One datastream, you would have to submit oracle votes on behalf of them which indicates some kind of agreement between you.
If such an agreement was in place, the 1:1 limitation would also have no effect as you could simply use another address to vote using your datastream but on behalf of someone else.

@joe-bowman
Copy link

However, the way I implemented this does not mean that someone could simply "subscribe" to another validator's datastream. The feeder/delegate has to intentionally send oracle votes for a specific validator.

Yes, indeed, hence:

which actually is not how this implementation appears to currently work; the 'centralised' - for lack of a better term - service would need to submit a vote for each operator

But that doesn't stop a single service provider becoming (albeit, consciously) disproportionately influential in the pricing oracle - business is business after all and if this becomes a source of revenue for a provider, it is in their interest to abuse this.

And in this position, the service provider becomes hugely powerful and if a malicious actor, is able to manipulate the market to their own end.

I appreciate that this is a worst case scenario, but it is not implausible. We should be tolerant to byzantine actors beyond consensus.

@hendrikhofstadt
Copy link
Contributor Author

However, the way I implemented this does not mean that someone could simply "subscribe" to another validator's datastream. The feeder/delegate has to intentionally send oracle votes for a specific validator.

Yes, indeed, hence:

which actually is not how this implementation appears to currently work; the 'centralised' - for lack of a better term - service would need to submit a vote for each operator

But that doesn't stop a single service provider becoming (albeit, consciously) disproportionately influential in the pricing oracle - business is business after all and if this becomes a source of revenue for a provider, it is in their interest to abuse this.

And in this position, the service provider becomes hugely powerful and if a malicious actor, is able to manipulate the market to their own end.

I appreciate that this is a worst case scenario, but it is not implausible. We should be tolerant to byzantine actors beyond consensus.

I think there's no way to prevent people from using a centralized service.
Even if you prevented voting delegation on the chain, any validator could still consume a premade API that gives you the best data for the vote.

Actually, the current system incentivizes such behaviour because only the validators that win the vote are rewarded.

Do you have any better idea to mitigate this?

IMO this change increases key security and does not worsen the already existing problem of potential datasource centralization.

@joe-bowman
Copy link

No way to prevent, no. But lets not make it easier for them :)

Maintaining a 1:1 mapping between delegatedFeeder and operator increases the overhead per validator for such a service provider. I can't see any benefit in allowing a single account to vote on behalf on multiple validators.

But I do agree, the mechanism is the fault here and I absolutely want the functionality to delegate votes to another address functionality; I brought this up with Do on day one - so thank you for implementing it, it massively increases key security 👍

As for improvements to the oracle, they are out of scope here, but I will think about it!

x/oracle/test_common.go Outdated Show resolved Hide resolved
@yun-yeo yun-yeo self-requested a review May 28, 2019 03:34
Copy link
Contributor

@yun-yeo yun-yeo left a comment

Choose a reason for hiding this comment

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

I fixed some build issue and tested cli in local chain. I also add rest interface and checked it.

@yun-yeo
Copy link
Contributor

yun-yeo commented May 28, 2019

Let's move other PR to solve centralized data source

@yun-yeo yun-yeo merged commit d7be4f7 into terra-money:develop May 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers must Mustfix for target release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants