tools: Allow specification of Reward Pool Account Balance in Genesis

[barnjamin]

Summary

Addresses #4642

Test Plan

Locally tested with sandbox and verified that rewards are not applied but more tests are probably warranted

[codecov]

Codecov Report

Merging #4643 (d4db850) into master (3f1f3e4) will increase coverage by 0.24%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4643      +/-   ##
==========================================
+ Coverage   54.39%   54.63%   +0.24%     
==========================================
  Files         403      403              
  Lines       51921    51932      +11     
==========================================
+ Hits        28240    28372     +132     
+ Misses      21300    21165     -135     
- Partials     2381     2395      +14     

Impacted Files	Coverage Δ
gen/walletData.go	0.00% <ø> (ø)
gen/generate.go	65.12% <100.00%> (+51.02%)	⬆️
ledger/tracker.go	74.04% <0.00%> (-3.83%)	⬇️
network/wsPeer.go	66.50% <0.00%> (-1.95%)	⬇️
ledger/acctupdates.go	70.19% <0.00%> (+0.59%)	⬆️
data/transactions/verify/txn.go	77.14% <0.00%> (+0.95%)	⬆️
catchup/peerSelector.go	100.00% <0.00%> (+1.04%)	⬆️
catchup/service.go	69.62% <0.00%> (+1.48%)	⬆️
crypto/merkletrie/node.go	93.48% <0.00%> (+1.86%)	⬆️
crypto/merkletrie/trie.go	68.61% <0.00%> (+2.18%)	⬆️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jannotti]

I like the idea. My hesitation is that devmode might be too broad. Is it true that if you're using devmode you definitely don't want rewards? A year ago, I would have said absolutely not, since it would be very reasonable to want to test your code with rewards coming in. Now? Yeah, maybe. It's not as though there are rewards on mainnet. (Do we have a rewards pool on testnet?) So I guess a dev barely cares if their code acts weird under rewards.

Anyway, I guess I would be 100% on board if this was controlled by an option in the relevant CLI.

[barnjamin]

I understand the point about devmode being a broad way to flag this off but I also can't come up a reason that someone running devmode would want rewards enabled since we've turned them off on MainNet.

I could see an argument for also making this change to the autogen'd genesis.json file with a !devmode private network config in the case that the dev needs things like real timestamps or block seed but also wants no rewards. I'm not sure the right way to handle that, though.

Maybe another flag in the Genesis struct? or some special value of rwd field?

[pbennett]

I talked to Ben about this the other day. There was a time where 'rewards' would've been on my mind, but I lost a legitimate 2 days or so trying to figure out why my system tests were randomly failing. I had hit a block threshold in tests where my balance checks (get balance X.. issue transactions.. txns + inner transactions should equal... balance Y) were failing. I'd remove some tests -it would work. It made no sense. I had other changes at the same time so I thought I had to have broken 'something'.. 'somewhere' but couldn't find it. I tore everything apart in various ways trying to track it down.

Finally.. I asked Ben if he knew of anyting weird re. devmode. He mentioned part. rewards and it was a head-smack moment. I've long since put part. rewards out of my mind - but I instantly knew it was the problem.

Since part. rewards are still part of the protocol, I'd be inclined to have this be an independent configuration parameter so it could be enabled if you wanted to simulate it (because they might start up again - but perhaps just for online accounts for eg). If it's too much of a short-term hassle, using devmode is probably sufficient and it can be changed later.

This definitely should be changed either way. These are the kinds of non-obvious problems that will cause problems for devs new to Algorand. I was extremely familiar w/ part. rewards and their issuance but even I completely forgot about them. Someone new to Algorand would be dumbfounded by this behavior - only present when using sandbox/etc.

[barnjamin]

Thinking about it more, does it make sense to key off proto version?

go-algorand/gen/generate.go

Line 75 in 345c99d

proto = genesisData.ConsensusProtocol

[barnjamin]

Added another bool flag to the gen.GenesisData struct to allow specification of the NoRewards condition.

I chose to name it as NoRewards so the default init will match the current behavior of enabled rewards.

I did not add any logic to the NetworkTemplate.Validate method, I'm not sure this would cause any templates to be invalid but if that isn't the case please lmk

[pbennett]

Added another bool flag to the gen.GenesisData struct to allow specification of the NoRewards condition.

So this would just be a new flag in the network template? Yeah, this would be great actually. I just have a network template as part of my docker container and it would be a trivial change. We just need to make sure this is well documented so users don't continue to run into it. Perhaps it becomes the default in the sandbox created template, but if manually created users could still leave rewards enabled if desired.

[brianolson]

LGTM. I think walling this off into a top level command line option separate from other effects makes it pretty safe. It'd be nice to get a double check from prior commentors. Is there a documentation change that should call out "hey, use this flag, it makes your dev net better" ?

[michaeldiamant]

@barnjamin Thanks for the initiative to identify a way forward here!

Take my approval lightly - I'm mostly unfamiliar with the space changed by the PR. I feel more comfortable with the added tests though I prefer a 2nd approval prior to merging.

[barnjamin]

Account balances after manual test in sandbox with 1000 pays w/ same snd/rcv for 0 algos.

Please ignore starting amount, the rewards field is the affected account field.

With patch && Genesis.RewardsBalance: 0

{'address': 'ANOKRWQFPMMKBEUAG644X5HTFX7N5BZF4U4Y7WK2KI5P3LVSFIH6IG745U', 'amount': 1999999999000000, 'amount-without-pending-rewards': 1999999999000000, 'apps-local-state': [], 'apps-total-schema': {'num-byte-slice': 0, 'num-uint': 0}, 'assets': [], 'created-apps': [], 'created-assets': [], 'min-balance': 100000, 'participation': {'selection-participation-key': 'NnBCnLEA+1vKojEdDf6udtaZ1E+vtpaJ0Z+3AN2r3GU=', 'state-proof-key': 'w1x/vC+OBBZKRIMUdCbVTLzYlFp+LwbWRtH9wNe6NOiucWTPGD3x1P0Ov/hvQriJbuRxNqab8fMR8bltRDkaDA==', 'vote-first-valid': 0, 'vote-key-dilution': 10000, 'vote-last-valid': 30000, 'vote-participation-key': 'ChBK7o9o1TFp9LG6Gl+o9RAxTwo+yp2CvSk6NKf8JKE='}, 'pending-rewards': 0, 'reward-base': 0, 'rewards': 0, 'round': 1000, 'status': 'Online', 'total-apps-opted-in': 0, 'total-assets-opted-in': 0, 'total-created-apps': 0, 'total-created-assets': 0}

With patch && unset Genesis.RewardsBalance:

{'address': 'EOD66G2YPR3PMEIK5CKLIHCCPKCRSDUIVGQLSUTXANBQCRLCCJ5TROJ6FQ', 'amount': 4000096000103978, 'amount-without-pending-rewards': 4000096000103978, 'apps-local-state': [], 'apps-total-schema': {'num-byte-slice': 0, 'num-uint': 0}, 'assets': [], 'created-apps': [], 'created-assets': [], 'min-balance': 100000, 'participation': {'selection-participation-key': 'BgOu9ayxCazT7wdFYDpdDLMm6QGCp3YJ1kPA+8sZq0w=', 'state-proof-key': 'wvhQVxi6bGX4OOGzC9tjIGHDOq4QoSZdfqGsXrxktp+H9XQU5ejApUqwoxPyaqXHPPPdfocdTkBWiNag749HXA==', 'vote-first-valid': 0, 'vote-key-dilution': 10000, 'vote-last-valid': 30000, 'vote-participation-key': 'tOPSz3PJNn3x0BElI9PKpf0qjBhVOC1Sanz04cJvcY8='}, 'pending-rewards': 0, 'reward-base': 24, 'rewards': 96001103978, 'round': 1000, 'status': 'Online', 'total-apps-opted-in': 0, 'total-assets-opted-in': 0, 'total-created-apps': 0, 'total-created-assets': 0}

Without Patch:

{'address': 'BCPM6P7PJT5RRA7SVSVKLIT6YYBYSKTOSJAM5VE6IRNHBSEHZKGEMGBQMQ', 'amount': 4000096000103978, 'amount-without-pending-rewards': 4000096000103978, 'apps-local-state': [], 'apps-total-schema': {'num-byte-slice': 0, 'num-uint': 0}, 'assets': [], 'created-apps': [], 'created-assets': [], 'min-balance': 100000, 'participation': {'selection-participation-key': 'DRmwzJI+dBSLzTyZU23kzY2qEzeFMefFlGcykqiaMr4=', 'state-proof-key': 'R+cf9N1SnUyrbxMI3DluqdkqruICxMuVBGI7Q8Y4liGVSRDGrTS4iTDJ5ckphSbsIAwQ7+cyTUmeGXNRqyEK4Q==', 'vote-first-valid': 0, 'vote-key-dilution': 10000, 'vote-last-valid': 30000, 'vote-participation-key': 'Brmi2od5h7QZ6uogIBWvR6Fxh7fWGYe2iDrdyK7vFSs='}, 'pending-rewards': 0, 'reward-base': 24, 'rewards': 96001103978, 'round': 1000, 'status': 'Online', 'total-apps-opted-in': 0, 'total-assets-opted-in': 0, 'total-created-apps': 0, 'total-created-assets': 0}```

[algorandskiy]

Can we somehow check if this generates the genesis for testnet as well?