Skip to content
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

Reward a predefined address a certain amount #1

Merged
merged 1 commit into from
May 23, 2022
Merged

Reward a predefined address a certain amount #1

merged 1 commit into from
May 23, 2022

Conversation

omerfirmak
Copy link
Contributor

This is the point that makes most sense to me for rewarding the commons budget, per block. Will test to see if this works in practice.

@hewison-chris
Copy link
Contributor

Nice, this looks like it could work.
Can you open PR to agora branch though. I should have mentioned before we will use it for our changes to start with.

@linked0
Copy link

linked0 commented May 17, 2022

Good, I will try to do unit tests with it. @hewison-chris, Don't we have to create a new branch on every PR with a forked repository?

@linked0
Copy link

linked0 commented May 17, 2022

Good start, but I think it could be better to remove the TODOs in this PR's code after adding the related commits.

@hewison-chris
Copy link
Contributor

hewison-chris commented May 17, 2022

Good start, but I think it could be better to remove the TODOs in this PR's code after adding the related commits.

I think that was the whole point of the TODO comments.
I think I just realized what you meant. Let's implement the TODOs before merging. However it can still be tested as is.

@omerfirmak
Copy link
Contributor Author

a miner node strangely bypasses this path, will try to move it elsewhere.

@omerfirmak
Copy link
Contributor Author

Those functions will not be called after the merge, see the updated code. It works fine

@omerfirmak omerfirmak changed the base branch from master to agora May 18, 2022 06:29
@omerfirmak
Copy link
Contributor Author

omerfirmak commented May 18, 2022

Tested to work fine, ready to review

@omerfirmak
Copy link
Contributor Author

Renamed commonsBudget config to commonsBudgetContract to emphasize that commons budget is expected to be a contract, as per @hewison-chris's request

Copy link
Contributor

@hewison-chris hewison-chris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

miner/worker.go Outdated Show resolved Hide resolved
@hewison-chris
Copy link
Contributor

Renamed commonsBudget config to commonsBudgetContract to emphasize that commons budget is expected to be a contract, as per @hewison-chris's request

Perhaps as it works for either just the comment update is ok.

@MichaelKim20
Copy link
Member

What is the rule that it is issued only for the first six years?

@hewison-chris
Copy link
Contributor

What is the rule that it is issued only for the first six years?

The white paper specifies that it should only reward the Commons Budget with 50 coins per 5 secs until 1_800_000_000 coins are rewarded. This is not yet implemented in this PR.

@MichaelKim20
Copy link
Member

Let me test it

  1. First, run the existing geth.
  2. Deploy the Smart Contract of the Commons Budget.
  3. Enter the address of the Smart Budget in the code of geth, build it, and run it.
  4. Verify that it is issued to the address of Commons Budget.

@MichaelKim20
Copy link
Member

#1 (comment)
@omerfirmak Could you test it in the same way as above?
Because we can't change the value made by BizNet.
Therefore, in order to fork later, the source must be modified and distributed.
My test result didn't go well. I entered the address in the source, but it doesn't apply well.

@MichaelKim20
Copy link
Member

@omerfirmak I changed the value of header.Coinbase to the address of the common budget in POA and it works well.
Can you check it for me?

@omerfirmak
Copy link
Contributor Author

@MichaelKim20 see the updated code, but dont change the source, just configure it using the genesis file.

example;

{
    "config": {
        "chainId": 2020,
        "homesteadBlock": 0,
        "eip150Block": 0,
        "eip155Block": 0,
        "eip158Block": 0,
        "byzantiumBlock": 0,
        "constantinopleBlock": 0,
        "petersburgBlock": 0,
        "istanbulBlock": 0,
        "berlinBlock": 0,
        "londonBlock": 0,
        "arrowglacierBlock": 0,
        "terminalTotalDifficulty": 20000000000000,
        "clique": {
          "period": 14,
          "epoch": 30000
        },
        "commonsBudget" : "0x087511AeF3B01d72C6c9f4F514560bc0577DF3bb",
        "commonsBudgetReward": 4,
        "lastCommonsBudgetRewardBlock": 50
    },
    "difficulty": "1",
    "gasLimit": "8000000",
    "extradata": "0x0000000000000000000000000000000000000000000000000000000000000000e79b1aeF1a4DD5E45F8D4B1CFABA71bedE94EF260000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
    "alloc": {
        "0xA87511AeF3B01d72C6c9f4F514560bc0577DF3bb" : { "balance": "542130130195846300000000000" },
        "0x82fFb1bB229552D020C88b9eE8D5e4042E6Cbd42" : { "balance": "100000000000000000000000000" },
        "0xC64edC529C17D593f5339E02C9055312cE0718B7" : { "balance": "100000000000000000000000000" }
      }
}

@MichaelKim20
Copy link
Member

MichaelKim20 commented May 20, 2022

I confirmed that the common budget is issued through the command below.
eth.getBalance("0x087511AeF3B01d72C6c9f4F514560bc0577DF3bb")

However, it is not visible in 'BlockScout'.
스크린샷 2022-05-21 오전 8 47 15

It would be better to create a different issue, right?

@MichaelKim20
Copy link
Member

MichaelKim20 commented May 20, 2022

When we open BizNet, the contents of genesis.json do not include items related to Commons Budget.
How can we add this params when we launch Agora's main net?

If you can make another issue and deal with it, This PR is great

@omerfirmak
Copy link
Contributor Author

I confirmed that the common budget is issued through the command below. eth.getBalance("0x087511AeF3B01d72C6c9f4F514560bc0577DF3bb")

However, it is not visible in 'BlockScout'. 스크린샷 2022-05-21 오전 8 47 15

It would be better to create a different issue, right?

That should be a problem on the explorer side. I bet if you send a TX to that address it will start to show just fine.

@omerfirmak
Copy link
Contributor Author

omerfirmak commented May 21, 2022

How can we add this params when we launch Agora's main net?

We can make it a scheduled hard fork in BizNet

@MichaelKim20
Copy link
Member

How can we add this params when we launch Agora's main net?

We can make it a scheduled hard fork in BizNet

Then I'd like to add one more parameter. I think it would be good to add the number of the block where the commons budget starts to be issued.
ex) startCommonsBudgetRewardBlock

Copy link
Member

@MichaelKim20 MichaelKim20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MichaelKim20
Copy link
Member

@omerfirmak I create a new issue and merge it.
#2

@omerfirmak
Copy link
Contributor Author

I confirmed that the common budget is issued through the command below. eth.getBalance("0x087511AeF3B01d72C6c9f4F514560bc0577DF3bb")

However, it is not visible in 'BlockScout'. 스크린샷 2022-05-21 오전 8 47 15

It would be better to create a different issue, right?

This seem to be a limitation on the blockscout part, it only updates balances on new TXs.

@mkykadir
Copy link

mkykadir commented Jun 8, 2022

I am testing this with local test-env but couldn't see the block rewards somehow

-- Tested with local build and singe node, it works; looking for why it didn't work in Docker net

@hewison-chris
Copy link
Contributor

I am testing this with local test-env but couldn't see the block rewards somehow

I think this was also mentioned here #1 (comment)
It might be a problem in the Explorer. Can you check if eth.getBalance shows the expected value?

@mkykadir
Copy link

mkykadir commented Jun 9, 2022

I am testing this with local test-env but couldn't see the block rewards somehow

I think this was also mentioned here #1 (comment) It might be a problem in the Explorer. Can you check if eth.getBalance shows the expected value?

Testing with getBalance, still returns 0 on all miners.
My bad, I was using geth without our changes to initialize our node folders from genesis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants