witnet / witnet-rust

Open source Rust implementation of the Witnet decentralized oracle protocol, including full node and wallet backend 👁️🦀
https://docs.witnet.io
GNU General Public License v3.0
179 stars 56 forks source link

feat(cli): validate that a validator is not already active using another withdrawer #2464

Closed drcpu-github closed 3 months ago

aesedepece commented 4 months ago

I'm just as supportive of adding this right now (to comply with the current WIP) as I'm supportive of removing it in a future 2.1 protocol revision if there's community consensus on it (as it facilitates delegation of stake).

The reason why I'm not pushing for dropping this restriction altogether is because allowing validators to operate on 3rd party stake is probably not that attractive anyway if onchain incentives are not provided. This is to say that when, at some point, we end up allowing that type of delegation, the protocol shall also allow the validators to take a fraction of the yield. If we go down that route, that percentage should be committed in the authorization message and in the stake transaction.

drcpu-github commented 4 months ago

I am somewhat confused by this statement. Are you implying that currently it should work to stake on the same validator with different withdrawers? I tested that and that does not seems to be the case.

It could be an observation bug with the query-stakes method rather than one in the staking logic itself. If I stake two times 10k on the same validator with two different withdrawers that seems to work (the network allows it) and I can see that both withdrawers have 10k staked using query-stakes. However, if I query the validator using query-stakes, there is only 10k balance.

I assumed this scenario was completely unsupported and hence the necessity for this validation (which also needs to happen at the block level).

Regardless, all of this would be much cleaner with a Stakes BTreeMap to query the withdrawer based on a validator (as discussed before), but that structure obviously becomes more complicated in case of true delegated staking.

drcpu-github commented 4 months ago

To debug above issue, I printed out the Stakes structure and verify what was wrong.

Stakes {
    by_key: {
        StakeKey { validator: PublicKeyHash { hash: [14, 15, 33, 0, 55, 170, 223, 182, 189, 89, 210, 80, 228, 65, 202, 209, 63, 245, 60, 37] }, withdrawer: PublicKeyHash { hash: [14, 15, 33, 0, 55, 170, 223, 182, 189, 89, 210, 80, 228, 65, 202, 209, 63, 245, 60, 37] } }:
            SyncStake { value: RwLock { data: Stake { coins: Wit(10000000000000), epochs: CapabilityMap { mining: 30, witnessing: 30 } } } },
        StakeKey { validator: PublicKeyHash { hash: [14, 15, 33, 0, 55, 170, 223, 182, 189, 89, 210, 80, 228, 65, 202, 209, 63, 245, 60, 37] }, withdrawer: PublicKeyHash { hash: [75, 251, 179, 224, 190, 2, 136, 139, 10, 32, 212, 136, 213, 25, 69, 129, 197, 211, 3, 230] } }:
            SyncStake { value: RwLock { data: Stake { coins: Wit(10000000000000), epochs: CapabilityMap { mining: 29, witnessing: 29 } } } }
    },
    by_validator: {
        PublicKeyHash { hash: [14, 15, 33, 0, 55, 170, 223, 182, 189, 89, 210, 80, 228, 65, 202, 209, 63, 245, 60, 37] }:
            SyncStake { value: RwLock { data: Stake { coins: Wit(10000000000000), epochs: CapabilityMap { mining: 30, witnessing: 30 } } } }
    },
    by_withdrawer: {
        PublicKeyHash { hash: [14, 15, 33, 0, 55, 170, 223, 182, 189, 89, 210, 80, 228, 65, 202, 209, 63, 245, 60, 37] }:
            SyncStake { value: RwLock { data: Stake { coins: Wit(10000000000000), epochs: CapabilityMap { mining: 30, witnessing: 30 } } } },
        PublicKeyHash { hash: [75, 251, 179, 224, 190, 2, 136, 139, 10, 32, 212, 136, 213, 25, 69, 129, 197, 211, 3, 230] }:
            SyncStake { value: RwLock { data: Stake { coins: Wit(10000000000000), epochs: CapabilityMap { mining: 29, witnessing: 29 } } } }
    },
    by_coins: {
        CoinsAndAddresses {
            coins: Wit(10000000000000),
            addresses: StakeKey { validator: PublicKeyHash { hash: [14, 15, 33, 0, 55, 170, 223, 182, 189, 89, 210, 80, 228, 65, 202, 209, 63, 245, 60, 37] }, withdrawer: PublicKeyHash { hash: [14, 15, 33, 0, 55, 170, 223, 182, 189, 89, 210, 80, 228, 65, 202, 209, 63, 245, 60, 37] } }
        }:
            SyncStake { value: RwLock { data: Stake { coins: Wit(10000000000000), epochs: CapabilityMap { mining: 30, witnessing: 30 } } } },
        CoinsAndAddresses {
            coins: Wit(10000000000000),
            addresses: StakeKey { validator: PublicKeyHash { hash: [14, 15, 33, 0, 55, 170, 223, 182, 189, 89, 210, 80, 228, 65, 202, 209, 63, 245, 60, 37] }, withdrawer: PublicKeyHash { hash: [75, 251, 179, 224, 190, 2, 136, 139, 10, 32, 212, 136, 213, 25, 69, 129, 197, 211, 3, 230] } }
        }:
            SyncStake { value: RwLock { data: Stake { coins: Wit(10000000000000), epochs: CapabilityMap { mining: 29, witnessing: 29 } } } }
    },
    minimum_stakeable: Some(Wit(10000000000000))
}

Everything looks good except the by_validator structure which only contains an entry for the latest 10k stake transaction to that validator. Looking at the code, this makes sense, there is no addition of coins per validator anywhere, just a replacement of the value associated with the validator key by a stake entry which is indexed a <validator, withdrawer> key pair. This is obviously not completely corrrect, but also seems rather irrelevant since this structure is used nowhere except by query-stakes, as far as I can tell.

drcpu-github commented 4 months ago

Superseded by https://github.com/witnet/witnet-rust/pull/2472/commits/059b6aa4a30b64cc9b30a573306e7d263073c00a.