tronprotocol / tips

TRON Improvement Proposals
231 stars 205 forks source link

TIP-542: Resource delegating supports customizable lock period #542

Closed tronenergymarket closed 1 year ago

tronenergymarket commented 1 year ago
tip: 542
Title: Resource delegating supports customizable lock period
Author: Tee <admin@tronenergy.market>
Discussions-to: https://github.com/tronprotocol/tips/issues/542
Status: Final
Type: Standards Track
Category: Core
Created: 2023-05-12

Simple Summary

Add a lock_period parameter in the wallet/delegateresource API in Stake 2.0, to support delegating bandwidth or energy resource with a specified lockup time.

Abstract

Besides the current API wallet/delegateresource that allows users to lockup a solid 3 days before the resouce could be undelegated, it is proposed to add a parameter lock_period in this API to support customizable lock period in resource delegating, with the unit of blocks. And the new resource delegating to the same address of same type should have this lock_period value larger than the lockup blocks left of the address.

Motivation

Currently in Stake 2.0, the lock parameter in the wallet/delegateresource is a bool type. If it is set to false, the delegated resources does not have a lockup time, it can be unstaked anytime after delegation. If it is set to true, the delegated resources cannot be undelegated within 3 days.

The solid lockup time of 3 days can not meet all the scenarios and is not conducive to the two parties to agree on a lease time in advance. If it can not be defined in the transaction, there is risk that the owner may undelegate the resource before the appointed time. Making the lockup time a customizable time will enhance the flexibility and security in delegation transactions.

Specifications

Modify wallet/delegateresource API, the user can call this API to delegate bandwidth or energy resource with a customized lockup time to another address.

Params:

  1. owner_address - Address of transaction initiator, data type is string
  2. receiver_address - Receiver address of resource to be delegated to, data type is string
  3. balance - Amount of TRX staked for resources to be delegated, unit is sun, data type is unit256
  4. resource - Resource type, "BANDWIDTH" or "ENERGY", data type is string
  5. lock - Whether it is locked. If it is set to false, there will not be any lockup time no matter what the lock_period value is. If it is set to true, the delegating transaction will have a lockup time, and the time is decided by the lock_period value.
  6. lock_period - The lockup period, unit is blocks, data type is int256, It indicates how many blocks the resource delegating is locked before it can be undelegated. If the lock time is not over, and the owner delegates the same type of resource with a new lockup time to the same address, the lockup time of all the resource of the same type will be reset to the new lockup time.That new lockup time cannot be less than (old lockup time + old transaction timestamp) - current time.
  7. visible - Whether the address is in base58 format, data type is bool

Returns: unsigned transaction, data type is JSON string

Example:

curl -X POST https://127.0.0.1:8092/wallet/delegateresource -d \
'{
  "owner_address":"TUoHaVjx7n5xz8LwPRDckgFrDWhMhuSuJM",     
  "receiver_address":"TZ4UXDV5ZhNW7fb2AMSbgfAEZ7hWsnYS2g",     
  "balance":1000000000,     
  "resource":"BANDWIDTH",     
  "lock": true,
  "lock_period": 86400,
  "visible":true
}'

Backward Compatibility

It will not influence the existed staking and unstaking related transactions.

If the lock is set to true, and lock_period is set to 0, or left blank, it also have the same consequence with that the lock_period is set to 86400.

Ademillery commented 1 year ago

I like it, just wonder whether it can be achieved. The first problem is that if you change the bool type to int256, it won't be backward compatible as said in the last section. The way I think it works is that add a new parameter in the API, for example "time", if the lock is true, then the lockup time is the value of "time". Please consider about it.

tronenergymarket commented 1 year ago

You can be totally right. And probably that's how it's gonna be because it feels much more easy to addapt. It depends on how the logic is addapted and how the variable is stored.

I purpose this way just in case it can be optimized directly to avoid that extra variable. Let's see what the dev team have to say.

lxcmyf commented 1 year ago

Thank you for the issue, I have read it and here are some words from my tech point of view:

  1. An extra variable is necessary, so that the transaction can be consistent with previous on the chain.
  2. The lockup time may be customized, but there should be limitation. Otherwise, it may cause too much work load for the network, and even worse the blockchain may be at risk of being attacked.

So do you have any detailed requirement of delegation lockup time range based on the delegating scenarios ?so I will assess whether the extra work load is fine.

Jamestepfoward commented 1 year ago

I think the method you are discussing is to somehow follow Stake 1.0, meaning that when initiating a new delegation to the same address, you try to merge the transaction and refresh the lockup time.

Well, how about this new approach: every delegating transaction accounts for one independent delegation. I mean even delegating to the same address, each delegating transaction has its own lockup time, and when undelegating, it's also each by each. I think it's more convenient and flexible for users in energy market. What would you say?

adianguo commented 1 year ago

I think the Stake2.0 should optimize the following points now:

  1. Add a parameter to customize the lock time. When it is set to lock, this parameter will take effect, and when it is set to not lock, this parameter will be invalid.
  2. every delegating transaction accounts for one independent delegation. I mean even delegating to the same address, each delegating transaction has its own lockup time, and when undelegating, it's also each by each. I think it's more convenient and flexible for users in energy market.
  3. It can be automatically unlocked after the expiration of the lock without manual unlocking, which should be the meaning of setting the lock
  4. Add wallet/cancelallunfreezev2 API, the user can call this API to cancel all the ongoing unstaking transactions in the waiting period.
lxcmyf commented 1 year ago

I think the Stake2.0 should optimize the following points now:

  1. Add a parameter to customize the lock time. When it is set to lock, this parameter will take effect, and when it is set to not lock, this parameter will be invalid.
  2. every delegating transaction accounts for one independent delegation. I mean even delegating to the same address, each delegating transaction has its own lockup time, and when undelegating, it's also each by each. I think it's more convenient and flexible for users in energy market.
  3. It can be automatically unlocked after the expiration of the lock without manual unlocking, which should be the meaning of setting the lock
  4. Add wallet/cancelallunfreezev2 API, the user can call this API to cancel all the ongoing unstaking transactions in the waiting period.

It sounds achievable, but please consider the scenario about making multiple delegates to the same person in a short time. In this case, the system needs to maintain the lock period for each delegating seperately, which will complicate the resources management for the same person or address. The system needs to create multiple key-value pairs according to each lock periods of the delegatings to maintain, or the key remains unchanged, and the value data structure needs to be modified to store multiple resource delegatings. This seems feasible, but if the number of delegating to a particular address is not limited, the data stored will be inflated infinitely, therefore, the performance of the database will be severely degraded.

Jamestepfoward commented 1 year ago

thank you @lxcmyf for the info

I think the Stake2.0 should optimize the following points now:

  1. Add a parameter to customize the lock time. When it is set to lock, this parameter will take effect, and when it is set to not lock, this parameter will be invalid.
  2. every delegating transaction accounts for one independent delegation. I mean even delegating to the same address, each delegating transaction has its own lockup time, and when undelegating, it's also each by each. I think it's more convenient and flexible for users in energy market.
  3. It can be automatically unlocked after the expiration of the lock without manual unlocking, which should be the meaning of setting the lock
  4. Add wallet/cancelallunfreezev2 API, the user can call this API to cancel all the ongoing unstaking transactions in the waiting period.

Good summary, and somehow I think it can be refined:

tronenergymarket commented 1 year ago

I will add my pov here:

It's better to maintain 1 lock amount like it is now (or 2 counting the non-timed ones), allowing unlimited delegations increasing the timer than limit the amount of delegates which can be done to an address. That would add another complex layer to the way the resources are managed in the market right now.


Answering @lxcmyf about this:

The lockup time may be customized, but there should be limitation. Otherwise, it may cause too much work load for the network, and even worse the blockchain may be at risk of being attacked.

I'm not sure why it should be exactly a problem.

The timer is just a passive variable (final timestamp) which doesn't need to have any load impact until someone tries to undelegate it, where the time is checked. I don't see the attack point here sincerely since the system doesn't need to do anything different that what it does right now with the 3 days which must be set internally.

Maybe there must be some clarification of how timer is working currently that I'm not taking into account.

Thank you in advance.

deiva89 commented 1 year ago

I think it is really useful and necessary, because right now it's almost impossible for energy providers to hold multiple permissions at the same time for multiple platforms. Nowadays there are platforms undelegating orders that have not placed themselves but other platform, leaving a customer without the energy for the time he actually paid for. If we can lock the order for the entire period this problem would be considerably reduced.

lxcmyf commented 1 year ago

It seems there are two options,

  1. make multiple delegatings to the same address, each has its own lock period, and undelegating would be done respectively
  2. there could be only one ongoing delegating to one certain address in the meantime, new delegating would refresh the lock period of the previous one, and undelegating remains as before

I think I need more suggestions and discussion on these two options to evaluate the pros and cons of them.

tronenergymarket commented 1 year ago
  1. The first option seams good at first sight cause each order has it's own lock, timer, etc. The issue with that is that to have something like that working there needs to be big tradeoffs which has been named here like having limited amount of delegations, etc which will make it not flexible enough and it would convert it a big resources consumption for the node itself which I feel it won't compensate neither to the nodes to have it.
  1. This second option sounds better to me as it has some strong points that I see useful:
    • It's simple to understand since you have only a single point to check.
    • There is no limits to the 1 to 1 delegations.
    • You can continue to undelegate partially one delegation which with version 1 it could be a probable restriction to remove complexity.

The big const I'm able to see is the freeze time which would maintain nearly the same essence of update the lock timer which is the reason why we are talking about version 1 to maintain those dates with their amounts.

lxcmyf commented 1 year ago
  1. The first option seams good at first sight cause each order has it's own lock, timer, etc. The issue with that is that to have something like that working there needs to be big tradeoffs which has been named here like having limited amount of delegations, etc which will make it not flexible enough and it would convert it a big resources consumption for the node itself which I feel it won't compensate neither to the nodes to have it.
  • It would be a big restriction for an energy buyer which constantly buys small amounts from 1 single seller if it has limits.
  • It would add another layer of complexity to sell resources.
  1. This second option sounds better to me as it has some strong points that I see useful:
  • It's simple to understand since you have only a single point to check.
  • There is no limits to the 1 to 1 delegations.
  • You can continue to undelegate partially one delegation which with version 1 it could be a probable restriction to remove complexity.

The big const I'm able to see is the freeze time which would maintain nearly the same essence of update the lock timer which is the reason why we are talking about version 1 to maintain those dates with their amounts.

For Option 2, if there is no limit to the lock in period, it can become infinitely large. If the user temporarily regrets without taking any other measures to modify this period, it will remain locked in and such a lock in will become meaningless. Therefore, I think it is necessary to specify a maximum period that is also within the user's acceptable range. Do you think so? Also, the unit of the lock in period as an API parameter should be days, right? The unit of seconds or less will make user operations more complex, which means the minimum lock period is one day. Within the program, days are converted into seconds.

Of course, further discussions will be held at TRON Developer Conference for both Option 1 and Option 2. Please actively participate at that time, thank you.

tronenergymarket commented 1 year ago

Hi, thank you for your answer @lxcmyf :

Of course, further discussions will be held at TRON Developer Conference for both Option 1 and Option 2. Please actively participate at that time, thank you.

Is this developer conference public? How can I participate? I would love to do it.

ZhangYiQiu commented 1 year ago

I think it is necessary to add a new parameter for the locking period. I think the unit of the new parameter can be minutes or hours, because seconds don't make much sense and no one will lock a few seconds. Also for compatibility with old data, when lock is true and the new parameter is 0, it should be locked for 3 days by default, and use its value when it has a value. For example:

{
  "owner_address": "Txxx1",
  "receiver_address": "Txxx2",
  "balance": 1000000,
  "resource": "ENERGY",
  "lock": true,
  "visible": true
}

It still means locked for 3 days.

{
  "owner_address": "Txxx1",
  "receiver_address": "Txxx2",
  "balance": 1000000,
  "resource": "ENERGY",
  "lock": true,
  "lock_time": 1,
  "visible": true
}

It means locked for 1 hour.

otakuinny commented 1 year ago
  1. Add a new optional parameter "duration" that accepts value in seconds. To keep it simple, this value should not be tied to the lock parameter. Whether lock is true or false, if duration parameter is passed, the delegation should be locked for that duration. The maximum allowed value for the duration parameter should be 2,592,000 secs (30 days). This way a rouge website can't lock away an unsuspecting user's trx for a long time. If duration parameter is not passed but the lock parameter is set to true, then the delegation should be locked for 3 days for backwards compatibility.
  2. Treating each locked delegation to the same receiver address as a separate delegation will have performance effects. No need to change this. When a new locked delegation is made to an address with a previous delegation, its lock time should be extended based on the following rules

    If the new delegation's expiration falls after the previous delegation's expiration time, then the new expiration time for all delegations should be the expiration time of the new delegation. E.g. If delegation 1 expires in 1 hour from now and delegation 2 is made with an expiration 3 hours from now, the new expiration of all the trx in delegation 1 and 2 should be 3 hours from now.

If the new delegation's expiration falls before the previous delegation's expiration time, then the expiration time for all trx should be the expiration time of the previous delegation.

Keep it simple and don't complicate things. All changes should be backwards compatible, meaning if no change is made to the dapp code after the update of this api, it should still work exactly like before.

While these changes are seen as improvements and are welcome, it looks pretty bad that these changes have to be made to a system so soon after its release. Makes whoever came up with the whole stake 2.0 model in the first place look like clown developers.

tronenergymarket commented 1 year ago

I think it is necessary to add a new parameter for the locking period. I think the unit of the new parameter can be minutes or hours, because seconds don't make much sense and no one will lock a few seconds. Also for compatibility with old data, when lock is true and the new parameter is 0, it should be locked for 3 days by default, and use its value when it has a value. For example:

{
  "owner_address": "Txxx1",
  "receiver_address": "Txxx2",
  "balance": 1000000,
  "resource": "ENERGY",
  "lock": true,
  "visible": true
}

It still means locked for 3 days.

{
  "owner_address": "Txxx1",
  "receiver_address": "Txxx2",
  "balance": 1000000,
  "resource": "ENERGY",
  "lock": true,
  "lock_time": 1,
  "visible": true
}

It means locked for 1 hour.

Hour granularity is fine to me (and I think for most). Just trying to get advance in future situations with seconds since hourly doesn't seems to be a standard measure if you know what I mean.

While these changes are seen as improvements and are welcome, it looks pretty bad that these changes have to be made to a system so soon after its release. Makes whoever came up with the whole stake 2.0 model in the first place look like clown developers.

For me personally I don't mind where the updates are done while they are done.

It's true that there is a gap between core devs and what community is doing right now which makes some decisions seems to be incompleted or not well thought.

Jamestepfoward commented 1 year ago

I think it is necessary to add a new parameter for the locking period. I think the unit of the new parameter can be minutes or hours, because seconds don't make much sense and no one will lock a few seconds. Also for compatibility with old data, when lock is true and the new parameter is 0, it should be locked for 3 days by default, and use its value when it has a value. For example:

{
  "owner_address": "Txxx1",
  "receiver_address": "Txxx2",
  "balance": 1000000,
  "resource": "ENERGY",
  "lock": true,
  "visible": true
}

It still means locked for 3 days.

{
  "owner_address": "Txxx1",
  "receiver_address": "Txxx2",
  "balance": 1000000,
  "resource": "ENERGY",
  "lock": true,
  "lock_time": 1,
  "visible": true
}

It means locked for 1 hour.

Hour granularity is fine to me (and I think for most). Just trying to get advance in future situations with seconds since hourly doesn't seems to be a standard measure if you know what I mean.

While these changes are seen as improvements and are welcome, it looks pretty bad that these changes have to be made to a system so soon after its release. Makes whoever came up with the whole stake 2.0 model in the first place look like clown developers.

For me personally I don't mind where the updates are done while they are done.

It's true that there is a gap between core devs and what community is doing right now which makes some decisions seems to be incompleted or not well thought.

Can't agree more, for every word.

lxcmyf commented 1 year ago

In the tron network, the concept of blocks is more deeply ingrained. A block generation takes 3 seconds, so the delegating lock period in blocks may be more in line with transaction rules and more acceptable to most people. Of course, blocks should have a maximum value limit, and it is recommended to set the number of blocks corresponding to 1 year or less time.

/wallet/delegateresource

Description: delegate resource

Params:

  1. owner_address - Address of transaction initiator, data type is string
  2. receiver_address - Receiver address of resource to be delegated to
  3. balance - Amount of TRX staked for resource to be delegated, unit is sun, data type is unit256
  4. resource - Resource type, "BANDWIDTH" or "ENERGY", data type is string
  5. lock - Whether it is locked, if it is set to true, and the proposal is not activated, the delegated resources cannot be undelegated within 3 days. if it is set to true and the proposal is activated, when lock_period=0, for compatibility with previous logic, it is still locked for three days.
  6. lock_period - When lock is true, this parameter is meaningful. Its unit is "block". When it is not 0, it indicates the duration of the lock corresponding to the number of blocks in time
  7. visible - Whether the address is in base58 format, data type is bool

Returns - unsigned transaction, data type is json string

Example:

curl -X POST  http://127.0.0.1:8092/wallet/delegateresource -d  \
'{"owner_address": "TJAVcszse667FmSNCwU2fm6DmfM5D4AyDh",
 "receiver_address": "TQ4gjjpAjLNnE67UFbmK5wVt5fzLfyEVs3",
 "balance": 1000000,
 "resource": "BANDWIDTH",
 "lock": true,
 "lock_period": 20,
 "visible": true
}'
otakuinny commented 1 year ago

In the tron network, the concept of blocks is more deeply ingrained. A block generation takes 3 seconds, so the delegating lock period in blocks may be more in line with transaction rules and more acceptable to most people. Of course, blocks should have a maximum value limit, and it is recommended to set the number of blocks corresponding to 1 year.

/wallet/delegateresource

Description: delegate resource

Params:

  1. owner_address - Address of transaction initiator, data type is string
  2. receiver_address - Receiver address of resource to be delegated to
  3. balance - Amount of TRX staked for resource to be delegated, unit is sun, data type is unit256
  4. resource - Resource type, "BANDWIDTH" or "ENERGY", data type is string
  5. lock - Whether it is locked, if it is set to true, and the proposal is not activated, the delegated resources cannot be undelegated within 3 days. if it is set to true and the proposal is activated, when lock_period=0, for compatibility with previous logic, it is still locked for three days.
  6. lock_period - When lock is true, this parameter is meaningful. Its unit is "block". When it is not 0, it indicates the duration of the lock corresponding to the number of blocks in time
  7. visible - Whether the address is in base58 format, data type is bool

Returns - unsigned transaction, data type is json string

Example:

curl -X POST  http://127.0.0.1:8092/wallet/delegateresource -d  \
'{"owner_address": "TJAVcszse667FmSNCwU2fm6DmfM5D4AyDh",
 "receiver_address": "TQ4gjjpAjLNnE67UFbmK5wVt5fzLfyEVs3",
 "balance": 1000000,
 "resource": "BANDWIDTH",
 "lock": true,
 "lock_period": 20,
 "visible": true
}'

1 year? Are you crazy??? It should be no more than 30 days. 1 Year will open so many attack vectors on user accounts that give permissions to websites.

The max value of lock_period should be 864,000 (30 days). When a value greater than 864,000 is passed, it should either throw an error or treat it as 0 which locks it for 3 days.

otakuinny commented 1 year ago

In the tron network, the concept of blocks is more deeply ingrained. A block generation takes 3 seconds, so the delegating lock period in blocks may be more in line with transaction rules and more acceptable to most people. Of course, blocks should have a maximum value limit, and it is recommended to set the number of blocks corresponding to 1 year.

/wallet/delegateresource

Description: delegate resource

Params:

  1. owner_address - Address of transaction initiator, data type is string
  2. receiver_address - Receiver address of resource to be delegated to
  3. balance - Amount of TRX staked for resource to be delegated, unit is sun, data type is unit256
  4. resource - Resource type, "BANDWIDTH" or "ENERGY", data type is string
  5. lock - Whether it is locked, if it is set to true, and the proposal is not activated, the delegated resources cannot be undelegated within 3 days. if it is set to true and the proposal is activated, when lock_period=0, for compatibility with previous logic, it is still locked for three days.
  6. lock_period - When lock is true, this parameter is meaningful. Its unit is "block". When it is not 0, it indicates the duration of the lock corresponding to the number of blocks in time
  7. visible - Whether the address is in base58 format, data type is bool

Returns - unsigned transaction, data type is json string

Example:

curl -X POST  http://127.0.0.1:8092/wallet/delegateresource -d  \
'{"owner_address": "TJAVcszse667FmSNCwU2fm6DmfM5D4AyDh",
 "receiver_address": "TQ4gjjpAjLNnE67UFbmK5wVt5fzLfyEVs3",
 "balance": 1000000,
 "resource": "BANDWIDTH",
 "lock": true,
 "lock_period": 20,
 "visible": true
}'

Remember, you are the one that came up with the specs for Stake 2.0, and the fact that we are making changes so soon shows that you are not very good at your job. So, how about you listen to the community this time (something you should have done the first time).

lxcmyf commented 1 year ago

In the tron network, the concept of blocks is more deeply ingrained. A block generation takes 3 seconds, so the delegating lock period in blocks may be more in line with transaction rules and more acceptable to most people. Of course, blocks should have a maximum value limit, and it is recommended to set the number of blocks corresponding to 1 year.

/wallet/delegateresource

Description: delegate resource Params:

  1. owner_address - Address of transaction initiator, data type is string
  2. receiver_address - Receiver address of resource to be delegated to
  3. balance - Amount of TRX staked for resource to be delegated, unit is sun, data type is unit256
  4. resource - Resource type, "BANDWIDTH" or "ENERGY", data type is string
  5. lock - Whether it is locked, if it is set to true, and the proposal is not activated, the delegated resources cannot be undelegated within 3 days. if it is set to true and the proposal is activated, when lock_period=0, for compatibility with previous logic, it is still locked for three days.
  6. lock_period - When lock is true, this parameter is meaningful. Its unit is "block". When it is not 0, it indicates the duration of the lock corresponding to the number of blocks in time
  7. visible - Whether the address is in base58 format, data type is bool

Returns - unsigned transaction, data type is json string Example:

curl -X POST  http://127.0.0.1:8092/wallet/delegateresource -d  \
'{"owner_address": "TJAVcszse667FmSNCwU2fm6DmfM5D4AyDh",
 "receiver_address": "TQ4gjjpAjLNnE67UFbmK5wVt5fzLfyEVs3",
 "balance": 1000000,
 "resource": "BANDWIDTH",
 "lock": true,
 "lock_period": 20,
 "visible": true
}'

1 year? Are you crazy??? It should be no more than 30 days. 1 Year will open so many attack vectors on user accounts that give permissions to websites.

The max value of lock_period should be 864,000 (30 days). When a value greater than 864,000 is passed, it should either throw an error or treat it as 0 which locks it for 3 days.

Thank you for these suggestions. Improving security awareness and identifying potential vulnerabilities are both valuable. Indeed, the one-year lock-period is too long, which leaves attackers too much time to launch other attacks. 30 days is a reasonable maximum, and this suggestion is worth adopting.

lxcmyf commented 1 year ago

In the tron network, the concept of blocks is more deeply ingrained. A block generation takes 3 seconds, so the delegating lock period in blocks may be more in line with transaction rules and more acceptable to most people. Of course, blocks should have a maximum value limit, and it is recommended to set the number of blocks corresponding to 1 year.

/wallet/delegateresource

Description: delegate resource Params:

  1. owner_address - Address of transaction initiator, data type is string
  2. receiver_address - Receiver address of resource to be delegated to
  3. balance - Amount of TRX staked for resource to be delegated, unit is sun, data type is unit256
  4. resource - Resource type, "BANDWIDTH" or "ENERGY", data type is string
  5. lock - Whether it is locked, if it is set to true, and the proposal is not activated, the delegated resources cannot be undelegated within 3 days. if it is set to true and the proposal is activated, when lock_period=0, for compatibility with previous logic, it is still locked for three days.
  6. lock_period - When lock is true, this parameter is meaningful. Its unit is "block". When it is not 0, it indicates the duration of the lock corresponding to the number of blocks in time
  7. visible - Whether the address is in base58 format, data type is bool

Returns - unsigned transaction, data type is json string Example:

curl -X POST  http://127.0.0.1:8092/wallet/delegateresource -d  \
'{"owner_address": "TJAVcszse667FmSNCwU2fm6DmfM5D4AyDh",
 "receiver_address": "TQ4gjjpAjLNnE67UFbmK5wVt5fzLfyEVs3",
 "balance": 1000000,
 "resource": "BANDWIDTH",
 "lock": true,
 "lock_period": 20,
 "visible": true
}'

1 year? Are you crazy??? It should be no more than 30 days. 1 Year will open so many attack vectors on user accounts that give permissions to websites. The max value of lock_period should be 864,000 (30 days). When a value greater than 864,000 is passed, it should either throw an error or treat it as 0 which locks it for 3 days.

Thank you for these suggestions. Improving security awareness and identifying potential vulnerabilities are both valuable. Indeed, the one-year lock-period is too long, which leaves attackers too much time to launch other attacks. 30 days is a reasonable maximum, and this suggestion is worth adopting.

Would it be better to set the lock_period as a parameter for proposal control? It is currently unclear how the majority of people tend to set this parameter, so it is up to everyone to vote when future proposals take effect.

tronenergymarket commented 1 year ago

It must be a parameter to allow updates in the future without having to update the node which is something all node sysadmin will appreciate 😄

otakuinny commented 1 year ago

Would it be better to set the period_period as a parameter for proposal control? It is currently unclear how the majority of people tend to set this parameter, so it is up to everyone to vote when future proposals take effect.

In the tron network, the concept of blocks is more deeply ingrained. A block generation takes 3 seconds, so the delegating lock period in blocks may be more in line with transaction rules and more acceptable to most people. Of course, blocks should have a maximum value limit, and it is recommended to set the number of blocks corresponding to 1 year.

/wallet/delegateresource

Description: delegate resource Params:

  1. owner_address - Address of transaction initiator, data type is string
  2. receiver_address - Receiver address of resource to be delegated to
  3. balance - Amount of TRX staked for resource to be delegated, unit is sun, data type is unit256
  4. resource - Resource type, "BANDWIDTH" or "ENERGY", data type is string
  5. lock - Whether it is locked, if it is set to true, and the proposal is not activated, the delegated resources cannot be undelegated within 3 days. if it is set to true and the proposal is activated, when lock_period=0, for compatibility with previous logic, it is still locked for three days.
  6. lock_period - When lock is true, this parameter is meaningful. Its unit is "block". When it is not 0, it indicates the duration of the lock corresponding to the number of blocks in time
  7. visible - Whether the address is in base58 format, data type is bool

Returns - unsigned transaction, data type is json string Example:

curl -X POST  http://127.0.0.1:8092/wallet/delegateresource -d  \
'{"owner_address": "TJAVcszse667FmSNCwU2fm6DmfM5D4AyDh",
 "receiver_address": "TQ4gjjpAjLNnE67UFbmK5wVt5fzLfyEVs3",
 "balance": 1000000,
 "resource": "BANDWIDTH",
 "lock": true,
 "lock_period": 20,
 "visible": true
}'

1 year? Are you crazy??? It should be no more than 30 days. 1 Year will open so many attack vectors on user accounts that give permissions to websites. The max value of lock_period should be 864,000 (30 days). When a value greater than 864,000 is passed, it should either throw an error or treat it as 0 which locks it for 3 days.

Thank you for these suggestions. Improving security awareness and identifying potential vulnerabilities are both valuable. Indeed, the one-year lock-period is too long, which leaves attackers too much time to launch other attacks. 30 days is a reasonable maximum, and this suggestion is worth adopting.

Would it be better to set the period_period as a parameter for proposal control? It is currently unclear how the majority of people tend to set this parameter, so it is up to everyone to vote when future proposals take effect.

yes, lock_period must be one of the chain parameters that can be updated through voting proposals with the initial default value as 864,000(30 days).

lxcmyf commented 1 year ago

Would it be better to set the period_period as a parameter for proposal control? It is currently unclear how the majority of people tend to set this parameter, so it is up to everyone to vote when future proposals take effect.

In the tron network, the concept of blocks is more deeply ingrained. A block generation takes 3 seconds, so the delegating lock period in blocks may be more in line with transaction rules and more acceptable to most people. Of course, blocks should have a maximum value limit, and it is recommended to set the number of blocks corresponding to 1 year.

/wallet/delegateresource

Description: delegate resource Params:

  1. owner_address - Address of transaction initiator, data type is string
  2. receiver_address - Receiver address of resource to be delegated to
  3. balance - Amount of TRX staked for resource to be delegated, unit is sun, data type is unit256
  4. resource - Resource type, "BANDWIDTH" or "ENERGY", data type is string
  5. lock - Whether it is locked, if it is set to true, and the proposal is not activated, the delegated resources cannot be undelegated within 3 days. if it is set to true and the proposal is activated, when lock_period=0, for compatibility with previous logic, it is still locked for three days.
  6. lock_period - When lock is true, this parameter is meaningful. Its unit is "block". When it is not 0, it indicates the duration of the lock corresponding to the number of blocks in time
  7. visible - Whether the address is in base58 format, data type is bool

Returns - unsigned transaction, data type is json string Example:

curl -X POST  http://127.0.0.1:8092/wallet/delegateresource -d  \
'{"owner_address": "TJAVcszse667FmSNCwU2fm6DmfM5D4AyDh",
 "receiver_address": "TQ4gjjpAjLNnE67UFbmK5wVt5fzLfyEVs3",
 "balance": 1000000,
 "resource": "BANDWIDTH",
 "lock": true,
 "lock_period": 20,
 "visible": true
}'

1 year? Are you crazy??? It should be no more than 30 days. 1 Year will open so many attack vectors on user accounts that give permissions to websites. The max value of lock_period should be 864,000 (30 days). When a value greater than 864,000 is passed, it should either throw an error or treat it as 0 which locks it for 3 days.

Thank you for these suggestions. Improving security awareness and identifying potential vulnerabilities are both valuable. Indeed, the one-year lock-period is too long, which leaves attackers too much time to launch other attacks. 30 days is a reasonable maximum, and this suggestion is worth adopting.

Would it be better to set the period_period as a parameter for proposal control? It is currently unclear how the majority of people tend to set this parameter, so it is up to everyone to vote when future proposals take effect.

yes, lock_period must be one of the chain parameters that can be updated through voting proposals with the initial default value as 864,000(30 days).

If the maximum of delegate lock-period is set to a proposal and later reduced, it may cause users who have already been delegated to no longer be able to delegate. Therefore, is it better to set this value to a constant? If the value of the proposal is not changed or increased in the future, it will be okay, but if it is reduced, there will be a problem. So if it is set as a proposal parameter, the initial value should be as small as possible(can be 30 days), and if the parameter is changed in the future, it can only be increased, not reduced.

LauraYH commented 1 year ago

What do you mean by 1 year opens too many attack vectors? This is a parameter-checking rule. You meant during that long time, pk has a better chance to be disclosed with a large amount of asset locking so that it leads to a higher chance to lose it?

In the tron network, the concept of blocks is more deeply ingrained. A block generation takes 3 seconds, so the delegating lock period in blocks may be more in line with transaction rules and more acceptable to most people. Of course, blocks should have a maximum value limit, and it is recommended to set the number of blocks corresponding to 1 year.

/wallet/delegateresource

Description: delegate resource Params:

  1. owner_address - Address of transaction initiator, data type is string
  2. receiver_address - Receiver address of resource to be delegated to
  3. balance - Amount of TRX staked for resource to be delegated, unit is sun, data type is unit256
  4. resource - Resource type, "BANDWIDTH" or "ENERGY", data type is string
  5. lock - Whether it is locked, if it is set to true, and the proposal is not activated, the delegated resources cannot be undelegated within 3 days. if it is set to true and the proposal is activated, when lock_period=0, for compatibility with previous logic, it is still locked for three days.
  6. lock_period - When lock is true, this parameter is meaningful. Its unit is "block". When it is not 0, it indicates the duration of the lock corresponding to the number of blocks in time
  7. visible - Whether the address is in base58 format, data type is bool

Returns - unsigned transaction, data type is json string Example:

curl -X POST  http://127.0.0.1:8092/wallet/delegateresource -d  \
'{"owner_address": "TJAVcszse667FmSNCwU2fm6DmfM5D4AyDh",
 "receiver_address": "TQ4gjjpAjLNnE67UFbmK5wVt5fzLfyEVs3",
 "balance": 1000000,
 "resource": "BANDWIDTH",
 "lock": true,
 "lock_period": 20,
 "visible": true
}'

1 year? Are you crazy??? It should be no more than 30 days. 1 Year will open so many attack vectors on user accounts that give permissions to websites.

The max value of lock_period should be 864,000 (30 days). When a value greater than 864,000 is passed, it should either throw an error or treat it as 0 which locks it for 3 days.

otakuinny commented 1 year ago

Would it be better to set the period_period as a parameter for proposal control? It is currently unclear how the majority of people tend to set this parameter, so it is up to everyone to vote when future proposals take effect.

In the tron network, the concept of blocks is more deeply ingrained. A block generation takes 3 seconds, so the delegating lock period in blocks may be more in line with transaction rules and more acceptable to most people. Of course, blocks should have a maximum value limit, and it is recommended to set the number of blocks corresponding to 1 year.

/wallet/delegateresource

Description: delegate resource Params:

  1. owner_address - Address of transaction initiator, data type is string
  2. receiver_address - Receiver address of resource to be delegated to
  3. balance - Amount of TRX staked for resource to be delegated, unit is sun, data type is unit256
  4. resource - Resource type, "BANDWIDTH" or "ENERGY", data type is string
  5. lock - Whether it is locked, if it is set to true, and the proposal is not activated, the delegated resources cannot be undelegated within 3 days. if it is set to true and the proposal is activated, when lock_period=0, for compatibility with previous logic, it is still locked for three days.
  6. lock_period - When lock is true, this parameter is meaningful. Its unit is "block". When it is not 0, it indicates the duration of the lock corresponding to the number of blocks in time
  7. visible - Whether the address is in base58 format, data type is bool

Returns - unsigned transaction, data type is json string Example:

curl -X POST  http://127.0.0.1:8092/wallet/delegateresource -d  \
'{"owner_address": "TJAVcszse667FmSNCwU2fm6DmfM5D4AyDh",
 "receiver_address": "TQ4gjjpAjLNnE67UFbmK5wVt5fzLfyEVs3",
 "balance": 1000000,
 "resource": "BANDWIDTH",
 "lock": true,
 "lock_period": 20,
 "visible": true
}'

1 year? Are you crazy??? It should be no more than 30 days. 1 Year will open so many attack vectors on user accounts that give permissions to websites. The max value of lock_period should be 864,000 (30 days). When a value greater than 864,000 is passed, it should either throw an error or treat it as 0 which locks it for 3 days.

Thank you for these suggestions. Improving security awareness and identifying potential vulnerabilities are both valuable. Indeed, the one-year lock-period is too long, which leaves attackers too much time to launch other attacks. 30 days is a reasonable maximum, and this suggestion is worth adopting.

Would it be better to set the period_period as a parameter for proposal control? It is currently unclear how the majority of people tend to set this parameter, so it is up to everyone to vote when future proposals take effect.

yes, lock_period must be one of the chain parameters that can be updated through voting proposals with the initial default value as 864,000(30 days).

If the maximum of delegate lock-period is set to a proposal and later reduced, it may cause users who have already been delegated to no longer be able to delegate. Therefore, is it better to set this value to a constant? If the value of the proposal is not changed or increased in the future, it will be okay, but if it is reduced, there will be a problem. So if it is set as a proposal parameter, the initial value should be as small as possible(can be 30 days), and if the parameter is changed in the future, it can only be increased, not reduced.

I think a maximum value of 30 days is safe enough to start and it should be ok to only ever increase this value through proposals. Because we are talking about "maximum" value which allows any lower value as lock period, I don't think we have to worry too much about this. I can't think of a situation where we might want to lower the maximum value below 30 days.

lxcmyf commented 1 year ago

Yes, we have verified this proposal and future modifications can only increase it.

vivian1912 commented 1 year ago

Close this issue as it is implemented by GreatVoyage-v4.7.2. Check TIP detail at TIP-542 Check implementation PR at https://github.com/tronprotocol/java-tron/pull/5255, https://github.com/tronprotocol/java-tron/pull/5260, https://github.com/tronprotocol/java-tron/pull/5264, https://github.com/tronprotocol/java-tron/pull/5267