Make BRI configurable by protocol version #4253

mitchelli · 2024-01-22T08:50:59Z

No description provided.

… Ceres

dincho · 2024-01-22T10:27:30Z

apps/aeutils/priv/aeternity_config_schema.json

@@ -1797,8 +1797,8 @@
                "protocol_beneficiaries" : {
                    "type" : "array",
                    "items" : {
-                        "description" : "Public keys belonging to protocol maintainers with reward shares (100 is 10%). If not set testnet or mainnet beneficiaries and shares will be used based on network_id configuration value. IMPORTANT: The value of this setting is under governance, thus it should not be changed without previous agreement within the configured network on doing so.",
-                        "pattern": "^ak_[1-9A-HJ-NP-Za-km-z]*:[0-9]+$"
+                        "description" : "Public keys belonging to protocol maintainers with reward shares (100 is 10%), optionally a protocol version can be specified from and/or to this account will be rewarded, e.g. ak_2KAcA2Pp1nrR8Wkt3FtCkReGzAi8vJ9Snxa4PcmrthVx8AhPe8:109:6: specifies from Ceres onwards, ak_2KAcA2Pp1nrR8Wkt3FtCkReGzAi8vJ9Snxa4PcmrthVx8AhPe8:109::5 specifies to Iris. If not set testnet or mainnet beneficiaries and shares will be used based on network_id configuration value. IMPORTANT: The value of this setting is under governance, thus it should not be changed without previous agreement within the configured network on doing so.",


What's the reason we need from-to spec? Usually protocol specific configuration requires just the protocol it kick s in, the from is somehow explicit?

The protocol_beneficiaries is a list of beneficiaries rather than a single beneficiary. If it was a single beneficiary then it would be implicit but with a list I don't think it is.

You could just have a single protocol version there and get the same behavio I think 🤔 - but, this is perhaps clearer (and easier to work with). @dincho since a BRI account is normally(?) active for multiple protocols it is a bit different.

Still, changing BRI is probably not the common case, so normal usage would be singleton list with Addr:BRIAmount:: I guess?

Yes, Addr:BRIAmount:: is the common case but you don't need to specify the to and from in this case, they are optional. You could just specify Addr:BRIAmount

From the code, it looks like it is possible to specify multi BRI accounts, it adds them together and makes sure they don't go above a certain limit.

okay, if there is technical requirement that's fine

Calling it a requirement is perhaps a bit strong, but I think this variant is slightly more fool-proof. Though I'm sure Nikita or Thomas would break it in a minute 😅

This is how a fool would try to change the beneficiary

Add NewAddr:1000 to config
That fails, because the old beneficiary is there and now the total is more than 100% ???

remove Addr:1000 from config....
That fails, I guess, because synchronizing and validating old blocks would no longer work?
Possibly not something one would notice straight away, but as soon as one starts that config on a new node to sync??

Introduce a new protocol vsn 2 and then configure
Addr:1000::1
NewAddr:1000::
Because someone said that that was the default use-case.
But it is actually better to say
NewAddr:1000:2:

Did I understand the documentation well?

apps/aecore/src/aec_dev_reward.erl

hanssv

Looks good to me!

I've run (locally) my hacky system test - where I configured a new BRI account at Ceres. And accounts are awarded as expected 👍

ThomasArts · 2024-01-25T09:34:38Z

apps/aecore/src/aec_dev_reward.erl

@@ -20,34 +21,47 @@
 -define(TOTAL_SHARES, 1000). % 100 shares == 10% of the reward
 %%% weighted avg BRI voting result of 1% to 20% (yes) votes is 10.89869526640124746202%, 109 of 1000 shares will be the protocol reward
 %%% for: "ak_2KAcA2Pp1nrR8Wkt3FtCkReGzAi8vJ9Snxa4PcmrthVx8AhPe8:109"
-define(MAINNET_BENEFICIARIES, [{<<172,241,128,85,116,104,119,143,197,105,4,192,224,207,200,138,230,84,111,38,89,33,239,21,201,183,185,209,19,60,109,136>>, 109}]).
+%%%%%%%%% TODO update with real values


Is this a TO DO for later or something that was planned for this PR?

This is for later, it should be before updated before Ceres goes live but we are waiting for a new BRI account from the foundation.

ThomasArts · 2024-01-25T09:52:16Z

apps/aecore/src/aec_dev_reward.erl

    catch
        error:Reason ->
            {error, Reason}
    end.

+beneficiaries_at_protocol(ProtocolVsn, Beneficiaries) ->


feels overly careful to check for FromProtocol and TOProtocol to be undefined.
The places where you call this function already guarantee that these are integers.

I would add a type spec that they must be integers and possibly crash if not by a guard is_integer in the function clause

The problem is that it is also called from the test function: set_beneficiaries which could include undefined as to or from parameters.

Yes, but that is test code... you don't need to expose this function from outside (only for testing) and therefore you can adapt the test to obtain clearer code and only export for test

The set_beneficiaries is only exported if TEST is set. It is used externally by the chain tests.

ThomasArts · 2024-01-25T10:13:43Z

apps/aeutils/priv/aeternity_config_schema.json

@@ -1797,8 +1797,8 @@
                "protocol_beneficiaries" : {
                    "type" : "array",
                    "items" : {
-                        "description" : "Public keys belonging to protocol maintainers with reward shares (100 is 10%). If not set testnet or mainnet beneficiaries and shares will be used based on network_id configuration value. IMPORTANT: The value of this setting is under governance, thus it should not be changed without previous agreement within the configured network on doing so.",
-                        "pattern": "^ak_[1-9A-HJ-NP-Za-km-z]*:[0-9]+$"
+                        "description" : "Public keys belonging to protocol maintainers with reward shares (100 is 10%), optionally a protocol version can be specified from and/or to this account will be rewarded, e.g. ak_2KAcA2Pp1nrR8Wkt3FtCkReGzAi8vJ9Snxa4PcmrthVx8AhPe8:109:6: specifies from Ceres onwards, ak_2KAcA2Pp1nrR8Wkt3FtCkReGzAi8vJ9Snxa4PcmrthVx8AhPe8:109::5 specifies to Iris. If not set testnet or mainnet beneficiaries and shares will be used based on network_id configuration value. IMPORTANT: The value of this setting is under governance, thus it should not be changed without previous agreement within the configured network on doing so.",


This is how a fool would try to change the beneficiary

Add NewAddr:1000 to config
That fails, because the old beneficiary is there and now the total is more than 100% ???

remove Addr:1000 from config....
That fails, I guess, because synchronizing and validating old blocks would no longer work?
Possibly not something one would notice straight away, but as soon as one starts that config on a new node to sync??

Introduce a new protocol vsn 2 and then configure
Addr:1000::1
NewAddr:1000::
Because someone said that that was the default use-case.
But it is actually better to say
NewAddr:1000:2:

Did I understand the documentation well?

mitchelli added 9 commits January 18, 2024 12:36

Make BRI account configurable by protocol

e1600b3

Fix aecore_pof & aehttp_integration

1d0805f

Override BRI because default stops at Iris, the hyper chains start at…

7f91fbe

… Ceres

Add test default values until the real BRI arrives

4749bcc

Exclude settings outside protocols

1ac4440

Fix aest_community_fork

e3e91b2

Test split of BRI accounts by protocol

5184ba7

Changed dev reward split by protocol test

7a8dcb4

Set back default BRI accounts

d7b6ca8

mitchelli self-assigned this Jan 22, 2024

mitchelli linked an issue Jan 22, 2024 that may be closed by this pull request

Make BRI configurable by protocol version #4246

Closed

mitchelli changed the title ~~Make BRI configurable~~ Make BRI configurable by protocol version Jan 22, 2024

Added some documentation to the schema

ec7402d

dincho reviewed Jan 22, 2024

View reviewed changes

hanssv reviewed Jan 22, 2024

View reviewed changes

apps/aecore/src/aec_dev_reward.erl Outdated Show resolved Hide resolved

apps/aecore/src/aec_dev_reward.erl Outdated Show resolved Hide resolved

Removed additional test BRI account and renamed unparse to encode.

2221aa0

hanssv approved these changes Jan 22, 2024

View reviewed changes

ThomasArts reviewed Jan 25, 2024

View reviewed changes

ThomasArts approved these changes Jan 25, 2024

View reviewed changes

mitchelli merged commit 2d5ce24 into master Jan 25, 2024
35 checks passed

mitchelli deleted the 4246-make-bri-configurable branch January 25, 2024 11:51

dincho added the kind/feature Issues or PRs related to a new feature label Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make BRI configurable by protocol version #4253

Make BRI configurable by protocol version #4253

mitchelli commented Jan 22, 2024

dincho Jan 22, 2024

mitchelli Jan 22, 2024

hanssv Jan 22, 2024

mitchelli Jan 22, 2024 •

edited

dincho Jan 22, 2024

hanssv Jan 22, 2024

ThomasArts Jan 25, 2024

hanssv left a comment

ThomasArts Jan 25, 2024

mitchelli Jan 25, 2024

ThomasArts Jan 25, 2024

mitchelli Jan 25, 2024

ThomasArts Jan 25, 2024

mitchelli Jan 25, 2024

ThomasArts Jan 25, 2024

Make BRI configurable by protocol version #4253

Make BRI configurable by protocol version #4253

Conversation

mitchelli commented Jan 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchelli Jan 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanssv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchelli Jan 22, 2024 •

edited