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.

Sign up for GitHub

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

Jump to bottom

Read-and-Write-actualGovernanceBlock-atomically #92

Merged
merged 1 commit into from
Jul 31, 2019
Merged

Read-and-Write-actualGovernanceBlock-atomically #92

merged 1 commit into from
Jul 31, 2019

Conversation

andybclee
Copy link
Contributor

Proposed changes

  • This PR is to fix data race on actualGovernanceBlock in governance object
  • Because the variable can be accessed by several goroutines, it have to be treated atomically

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

@andybclee andybclee self-assigned this Jul 30, 2019
ehnuje
ehnuje reviewed Jul 30, 2019
View reviewed changes
governance/default.go
@@ -570,14 +570,14 @@ func (g *Governance) initializeCache() error {
for _, v := range indices {
if num, data, err := g.ReadGovernance(v); err == nil {
g.itemCache.Add(getGovernanceCacheKey(num), data)
g.actualGovernanceBlock = num
atomic.StoreUint64(&g.actualGovernanceBlock, num)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it okay to use atomic.StoreUint64 and atomic.LoadUint64, instead of using atomic.Value for actualGovernanceBlock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ehnuje I think the name made you confused. actualGovernanceBlock is the number of the block and it has uint64 data type.

ehnuje
ehnuje approved these changes Jul 30, 2019
View reviewed changes
Copy link
Contributor

@ehnuje ehnuje left a comment

Choose a reason for hiding this comment

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

As we talked f2f, it seems fine for now. Let's make an issue to track whether to use atomic.Value here.

@andybclee andybclee mentioned this pull request Jul 30, 2019
Closed
@andybclee
Copy link
Contributor Author

As we talked f2f, it seems fine for now. Let's make an issue to track whether to use atomic.Value here.

Added #93 and https://groundx.atlassian.net/jira/software/projects/TB/boards/36/backlog?assignee=5ab9f16ca142832a9381c64a&selectedIssue=TB-382

@andybclee andybclee merged commit 77dcfde into klaytn:master Jul 31, 2019
@andybclee andybclee deleted the 0014 branch July 31, 2019 00:04
KimKyungup added a commit that referenced this pull request Jul 31, 2019
@KimKyungup
Service chain RC1 (#104)
3a371fd 
* SC : Refine bridge/token contract (#19)

* Refactor token contract for service chain

* Refactoring bridge/token contract

- token/nft -> ERC20/721
- base token contract
- remove unnecessary return value

* go generate nft.go

* refactor duplicated code

* SC : Support value transfer fee for KLAY/ERC20 (#23)

* Support KLAY/ERC20 tranfser with fee

* fee comsuption during request and add unit test

* Add wrong fee unit test

* revert unnecessary change

* reflected review

* fixed typo

* fixed typo

* reflect revie

* Replace smseo with kjhman21 in CODEOWNERS (#26)

Since smseo would not be able to actively participate in code reviews
for a while, it would be better to remove him from CODEOWNERS.

* SC: fix stopping bridge recoveries for shutdown (#22)

* SC: mitigate a scn shutdown problem (#25)

*  SC: add kspn and ksen binary support (#21)

* SC: add kspn and ksen binary support

* SC: fix command option handling

* SC: apply review comments

* SC: remove network configuration for service chain

* SC: remove baobab and cypress options from kspn and ksen

* SC: try rebuild

* SC: try rebuild

* Update cmd/ksen/doc.go

Co-Authored-By: Sangmin Seo (Sam) <sam.seo@groundx.xyz>

* Update build/rpm/main.go

Co-Authored-By: Sangmin Seo (Sam) <sam.seo@groundx.xyz>

* Update cmd/ksen/main.go

Co-Authored-By: Sangmin Seo (Sam) <sam.seo@groundx.xyz>

* Update cmd/kspn/doc.go

Co-Authored-By: Sangmin Seo (Sam) <sam.seo@groundx.xyz>

* Update cmd/kspn/main.go

Co-Authored-By: Sangmin Seo (Sam) <sam.seo@groundx.xyz>

* apply review comments for converting node types

* SC : Add value transfer Fee APIs (#29)

* Add SC Fee APIs

* add missed web3ext for new api

* SC: add subbridge support for ksen (#30)

* Added Service Chain Network on homi docker compose (#20)

* Added Service Chain Network on homi docker compose

- service chain network is added on docker compose

* Renamed serviceGenesis to scGenesis

* Change code owners (#28)

* SC: fix a mainbridge port mapping of docker compose (#34)

* SC: remove an address book from the sc genesis in homi (#36)

* SC: remove an address book from the sc genesis in homi

* SC: update homi docker compose for kspn and ksen (#35)

* SC: update homi docker compose for kspn and ksen

* SC: change the docker image name

* Update names

* Apply review comments

* SC: change the max peer of the main and sub bridges to one (#37)

* SC: change the max peer of the main and sub bridge to one

* remove deadcode

* apply review comments

* Revert "Prevent voting except add, remove validator (#3565)" (#31)

* [Reward] stakingInfo (#32)

make reward package under klaytn directory
copy stakingInfo from contracts/reward/reward.go to reward/stakingInfo.go
make governanceHelper interface
unused function string() from stakingInfo is not copied

* Add test code for checking generated hashes are equal to that of baobab and cypress (#41)

* SC: update parent receipt API for more information as RPC output (#33)

* Update parent receipt API for more information as RPC output

* reflected review

* review reflected

* Add nil checks for the wrong genesis configuration (#40)

* Add nil checks for the wrong genesis configuration

* Apply a review comment

* fix ci test fail (#44)

* increase delay time

* change default shell to bash

* remove unsuse testscript

* Add a godoc badge with descriptions (#48)

* account: add optional block number for contract calls (#46)

* account: add optional block number for calls

* Apply a review comment

* [Reward] Staking info cache (#42)

copy stakingInfoCache from contract/reward/reward.go to reward/stakingInfoCache.go
copy test code from reward_test.go to stakingInfoCache_test.go

* change log level (#52)

* Fix a readme for kspn (#51)

* SC: Support FeeLimit of value transferring. (#38)

* Support FeeLimit

* reflect of Aidan's review.

* reflect review

- modifiy RequestKLAYTransfer parameter
FeeLimit -> Amount
- prevent to usless KLAY/ERC transfer to refund zero value.

* fix argument type

* fix typo

* remove zero transfer check

* reflected review

* reflect review

* simplify code

* [Reward] rewardConfigCache (#47)

add rewardConfigCache and test code

* Update doc.go for governance package (#45)

* Update doc.go for governance package

* SC : Refactor Request/Handle event structure (#54)

* refactor event

* clean code

* clean code2

* reflected review

* reflect review

* Changed GovernanceVotes GovernanceTallies and GovernanceSet (#43)

* [Reward] AddressBookManager (#53)

Add addressBookManager and test code

* [reward] doc.go (#50)

update reward/doc.go

* Update doc.go of package downloader (#62)

* write doc

* Update datasync/downloader/doc.go

Co-Authored-By: Ethan (Kyungup Kim) <ethan.kim@groundx.xyz>

* Apply suggestions from code review

Co-Authored-By: Jeongho Albert Nah <jeongho.nah@gmail.com>
Co-Authored-By: Ethan (Kyungup Kim) <ethan.kim@groundx.xyz>

* Apply suggestions from code review

* Apply suggestions from code review

Co-Authored-By: Melvin Junhee Woo <melvin.woo@groundx.xyz>
Co-Authored-By: Jasmine <32922423+jimni1222@users.noreply.github.com>

* SC : Add extended bridge contract / callback contract example (#57)

* Service chain callBack example

* go fmt

* reflect review

* rebase branch and go generate caused by contract code changed

* reflect suggestions

* update reward/doc.go (#69)

update reward/doc.go

* Add description of doc.go for common/hexutil (#67)

* Add more description for common/hexutil package on doc.go
Co-Authored-By: Melvin Junhee Woo <melvin.woo@groundx.xyz>

* Fix typo and refine a sentence in README.md (#65)


Signed-off-by: Hyung-Kyu Choi <hqueue.choi@groundx.xyz>

* [Reward] Staking manager (#60)

add stakingManager and test code.

* database: updated doc.go (#71)

* database: updated doc.go

* Fixed gofmt failure

* Reflected review comments

* Apply suggestions from code review

Co-Authored-By: Rick <rick.no@groundx.xyz>

* SC : Refactor bridge contract method parameter (#59)

* refactor contract method parameter

* reflect review

* reflect review

* Add more description for common/profile package (#75)

* Add descriptions on doc.go for common/compiler package (#64)

* Add descriptions on doc.go for common/compiler package
Co-Authored-By: Jasmine <32922423+jimni1222@users.noreply.github.com>

* Apply review feedbacks (#70)

* Update doc.go of package account (#56)

* Update doc.go of package account

* Remove line

* Reflect review

* Add period

* Edit mistypo

* Update blockchain/types/account/doc.go

Co-Authored-By: Jeongho Albert Nah <jeongho.nah@gmail.com>

* Update blockchain/types/account/doc.go

Co-Authored-By: Jeongho Albert Nah <jeongho.nah@gmail.com>

* fmt

* Make governance variables to be atomic (#61)

* Add missing atomic operation for lastGovernanceStateBlock
Make nodeAddress as an atomic.Value
Add idxCacheLock to protect idxCache

* Update doc.go of package fetcher  (#68)

* update doc

* Apply suggestions from code review

Co-Authored-By: Melvin Junhee Woo <melvin.woo@groundx.xyz>

* Apply suggestions from code review

Co-Authored-By: Jasmine <32922423+jimni1222@users.noreply.github.com>

* Apply suggestions from code review

Co-Authored-By: Jasmine <32922423+jimni1222@users.noreply.github.com>

* Add more description for common/math package (#72)

* Add more description for common/math package

Co-Authored-By: Melvin Junhee Woo <melvin.woo@groundx.xyz>

Co-Authored-By: Ethan (Kyungup Kim) <ethan.kim@groundx.xyz>

* [Reward] Reward Distributor (#74)

add RewardDistributor and test code.

* Change the issue report location from Ethereum to Klaytn (#85)

* [Reward] Addressbook connector (#86)

* change addressBookManager to addressBookConnector
* change testcode

* change file names to snake_case (#87)

this pr changes names of files in reward package to snake_case

* SC: Support keystore files for bridge accounts (#77)

* support keystore for bridge accounts

* reflect review @aidan @jasmine

* reflect review 2nd

* remove unnecessary flag test case

* reflect review #3

* fix typo

* review #4

* review #5

* go fmt

* remove unnecessary.

* review 6

* Update blockchain/types/accountkey/doc.go (#55)

* Update blockchain/types/accountkey/doc.go

* Modify form

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Donghwan Aidan Kwon <48199072+aidan-kwon@users.noreply.github.com>

* Add period

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Jeongho Albert Nah <jeongho.nah@gmail.com>

* Reflect review

* Add fail key description

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Ethan (Kyungup Kim) <ethan.kim@groundx.xyz>

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Jeongho Albert Nah <jeongho.nah@gmail.com>

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Jeongho Albert Nah <jeongho.nah@gmail.com>

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Jeongho Albert Nah <jeongho.nah@gmail.com>

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Donghwan Aidan Kwon <48199072+aidan-kwon@users.noreply.github.com>

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Donghwan Aidan Kwon <48199072+aidan-kwon@users.noreply.github.com>

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Donghwan Aidan Kwon <48199072+aidan-kwon@users.noreply.github.com>

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Donghwan Aidan Kwon <48199072+aidan-kwon@users.noreply.github.com>

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Donghwan Aidan Kwon <48199072+aidan-kwon@users.noreply.github.com>

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Donghwan Aidan Kwon <48199072+aidan-kwon@users.noreply.github.com>

* Update blockchain/types/accountkey/doc.go

* Update blockchain/types/accountkey/doc.go

* Update blockchain/types/accountkey/doc.go

* Update blockchain/types/accountkey/doc.go

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Donghwan Aidan Kwon <48199072+aidan-kwon@users.noreply.github.com>

* Update blockchain/types/accountkey/doc.go

Co-Authored-By: Donghwan Aidan Kwon <48199072+aidan-kwon@users.noreply.github.com>

* Update blockchain/types/accountkey/doc.go

* fmt

* [doc] update fork/doc.go (#91)

update fork/doc.go

* state: updated blockchain/state/doc.go (#81)

* Updated blockchain/state/doc.go

* Fixed gofmt failure

* Reflected review comments

* Reflected review comments

* Reflected review comments

* Fixed for godoc

* Update blockchain/state/doc.go

Co-Authored-By: Donghwan Aidan Kwon <48199072+aidan-kwon@users.noreply.github.com>

* Add more description for web3ext.go (#89)

* Add more description for web3ext.go

* Update console/web3ext/doc.go

Co-Authored-By: Melvin Junhee Woo <melvin.woo@groundx.xyz>

* Add more expression on doc.go for bitutil package (#63)

* Add more expression on doc.go for bitutil package

* Update common/bitutil/doc.go

Co-Authored-By: Melvin Junhee Woo <melvin.woo@groundx.xyz>

* Refactor voteMap to avoid data race (#83)

* Refactor voteMap to avoid data race

* Apply feedbacks

* statedb: updated storage/statedb/doc.go (#82)

* statedb: updated storage/statedb/doc.go

* Reflected review comments

* SecureTire -> SecureTrie

* Changed to meet the requirements of godoc formatting

* Read-and-Write-actualGovernanceBlock-atomically (#92)

* Add more description on doc.go for common/fdlimt package (#66)

* Add more description on doc.go for common/fdlimt package
Co-Authored-By: Melvin Junhee Woo <melvin.woo@groundx.xyz>

* Merge duplicated function GetLatestGovernanceItem and GetGovernanceValue (#94)

* Merge-duplicated-function-GetLatestGovernanceItem
* Remove GetLatestGovernanceItem()

* Add more description for common/mclock package (#73)

* Add more description for common/mclock package
Co-Authored-By: Ethan (Kyungup Kim) <ethan.kim@groundx.xyz>
Co-Authored-By: Melvin Junhee Woo <melvin.woo@groundx.xyz>

* Add more description on doc.go of the `common` package (#88)

* Add more description on doc.go of the `common` package
Co-Authored-By: Melvin Junhee Woo <melvin.woo@groundx.xyz>
Co-Authored-By: Kuwon Sebastian Ra <kwla103@naver.com>

* SC: add a multi-bridge and multi-sig support (#79)

* change default consensus of SC to istanbul (#99)

* SC: Added handledRequestValueTransfer<hash, isDone> map into bridge contract. (#78)

* Add RTX Hash into handleValueTransfer

* Added TCs to check the feature

* update handledRequests as an base contract.

* review

* review

* reflect review #3

* homi support to generate keystore

* review#1

* review #1-1

* review#1-2

* review#1-3 @aidan

* review#1-4 @melvin
@andybclee andybclee mentioned this pull request Aug 9, 2019
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehnuje ehnuje ehnuje approved these changes

@jeonghonah jeonghonah jeonghonah approved these changes

@aidan-kwon aidan-kwon Awaiting requested review from aidan-kwon

@jimni1222 jimni1222 Awaiting requested review from jimni1222

@KimKyungup KimKyungup Awaiting requested review from KimKyungup

@laggu laggu Awaiting requested review from laggu

@nogang nogang Awaiting requested review from nogang

Assignees

@andybclee andybclee

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@andybclee @ehnuje @jeonghonah