Change PublicKey on Bid #4566
Change PublicKey on Bid #4566
Conversation
wojcik91
force-pushed
the
validator_transfer
branch
from
1ea90ac
to
cc38935
Compare
EdHastingsCasperAssociation
changed the title
wojcik91
force-pushed
the
validator_transfer
branch
from
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.
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.
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.
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.
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.
Hi @mpapierski.
I've introduced the changes we've discussed:
- bumped the cost of the new entrypoint (to twice the cost of
add_bidfor now) - removed recursion in favor of a
forloop with a limited number of iterations
wojcik91
force-pushed
the
validator_transfer
branch
2 times, most recently
from
5ca329c
to
fbf23ce
Compare
wojcik91
force-pushed
the
validator_transfer
branch
from
fbf23ce
to
be29059
Compare
wojcik91
force-pushed
the
validator_transfer
branch
from
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.
as a side note, in tests we prefer to use .expect("message of what was expected") instead of .unwrap()
|
bors r+ |
|
Build succeeded: |
casperlabs-bors-ng
bot
merged commit 8a56be6
into
casper-network:feat-2.0
|
Congrats! |
Implements casper-network/ceps#91