-
Notifications
You must be signed in to change notification settings - Fork 222
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
Change PublicKey on Bid #4566
Change PublicKey on Bid #4566
Conversation
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.
The implementation looks good overall. Assuming my two requests for change are addressed, I will approve.
1ea90ac
to
cc38935
Compare
a9919d3
to
ec19788
Compare
@EdHastingsCasperAssociation I reworked the implementation to use the new In the GS update tool I took a bit of a shortcut and assumed that |
@mpapierski ping |
storage/src/system/auction/detail.rs
Outdated
if !bid_key.is_bid_addr_key() { | ||
return Err(Error::InvalidKeyVariant); | ||
} | ||
match provider.read_bid(bid_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.
Can we change this logic to a loop with a fixed upper bound of iteration count instead of recursion (this, and the other place that uses loop
)? Even with the guard rails in place at the entry point, I believe you could still potentially abuse this: create a long enough chain of validator entries that point at each other. I.e. A has a bridge record to B, B has a bridge record to C, etc.
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.
What do you think would be a reasonable iteration limit?
I expect we can safely assume that actual bids won't form very long chains, so there's no need for additional validation when creating the bridge record. Is that ok?
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.
What do you think would be a reasonable iteration limit?
Considering that's just so we don't do possibly unbounded computation, I think 10 should be more than enough. If there's a legitimate use case that you have to change the keys multiple times within an era then it should serve the purpose
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.
I think the longest possible chain could happen when processing unbonds (due to the length of time between creating and processing the UnbondingPurse
), but I guess it's hard to see a valid use-case for transfering a bid record that often, so 10 iterations should suffice. We'll just have to be clear about this limitation in the documentation.
On the other hand I'm a bit worried about allowing the user to create an unprocessable entity. Is there a precedent for this in the codebase?
While figuring out if it's possible to create a loop in bridge records I also realized there's another issue with our approach - since bridge records are meant to reuse the same address as ValidatorBid
it's currently not possible to transfer a bid back to the original address unless the bridge record is pruned. Is this something we should consider as a bug or leave it as a known limitation?
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.
Hi @mpapierski.
I've introduced the changes we've discussed:
- bumped the cost of the new entrypoint (to twice the cost of
add_bid
for now) - removed recursion in favor of a
for
loop with a limited number of iterations
5ca329c
to
fbf23ce
Compare
fbf23ce
to
be29059
Compare
9c411b6
to
ca49edd
Compare
bors r+ |
4566: Change PublicKey on Bid r=EdHastingsCasperAssociation a=wojcik91 Implements casper-network/ceps#91 4657: Prevent stray `accounts.toml` files from breaking upgrades r=EdHastingsCasperAssociation a=fizyk20 The rewards calculation used to take the validators from `accounts.toml` when writing zero rewards to validators at genesis or at an upgrade point. This is now just replaced with an empty map, which has the same effect and doesn't depend on external data. Co-authored-by: Maciej Wójcik <[email protected]> Co-authored-by: Bartłomiej Kamiński <[email protected]>
Build failed (retrying...): |
4566: Change PublicKey on Bid r=EdHastingsCasperAssociation a=wojcik91 Implements casper-network/ceps#91 Co-authored-by: Maciej Wójcik <[email protected]>
Build failed: |
update.assert_written_purse_is_unit(bid_write.bonding_purse().unwrap()); | ||
update.assert_written_balance(bid_write.bonding_purse().unwrap(), 102); |
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.
as a side note, in tests we prefer to use .expect("message of what was expected") instead of .unwrap()
bors r+ |
Build succeeded: |
8a56be6
into
casper-network:feat-2.0
Congrats! |
Implements casper-network/ceps#91