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

core: create a header chain structure shared by full and light clients #2081

Merged
merged 1 commit into from Mar 10, 2016

Conversation

Projects
None yet
6 participants
@zsfelfoldi
Copy link
Contributor

zsfelfoldi commented Dec 16, 2015

This PR moves the basic canonical chain logic out of core.BlockChain into core.HeaderChain, which only handles block headers. The purpose of this refactoring is to separate everything that is actually related to a full node from the basic chain logic which can be used by the light client (the future light.LightChain structure) too.

@robotally

This comment has been minimized.

Copy link

robotally commented Dec 16, 2015

Vote Count Reviewers
👍 2 @fjl @karalabe
👎 0

Updated: Mon Mar 7 10:51:06 UTC 2016

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Dec 16, 2015

Please keep HeaderChain in package core. It is fine if package light depends on core.

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-chain branch Dec 16, 2015

@zsfelfoldi zsfelfoldi changed the title core/hdrchain: create a header chain structure shared by core.BlockChain and light.LightChain core: create a header chain structure shared by core.BlockChain and light.LightChain Dec 16, 2015

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-chain branch Dec 16, 2015

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 16, 2015

Current coverage is 50.88%

Merging #2081 into develop will decrease coverage by -0.01% as of 36e6b8b

Powered by Codecov. Updated on successful CI builds.

@zsfelfoldi zsfelfoldi added pr:review and removed in progress labels Dec 17, 2015

@zsfelfoldi zsfelfoldi changed the title core: create a header chain structure shared by core.BlockChain and light.LightChain core: create a header chain structure shared by full and light clients Dec 17, 2015

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-chain branch Dec 17, 2015

@zsfelfoldi zsfelfoldi added in progress and removed pr:review labels Jan 11, 2016

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-chain branch 2 times, most recently Jan 11, 2016

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-chain branch to 8425266 Jan 12, 2016

@zsfelfoldi zsfelfoldi added pr:review and removed in progress labels Jan 12, 2016

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Feb 2, 2016

Is this PR still up for review, or was it superseded by #2120?

@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Feb 2, 2016

@karalabe #2081 is the first commit of #2120. If you find an issue with the header chain, I'd say comment it here.

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Feb 4, 2016

A few immediate problems that require some larger effort:

  • As you've introduced a standalone header chain, please write an entire test suite that just tests the header chain itself. I mean every operation.
  • Doing so, you'll discover that the header chain is very very racey, as it assumes that it's embedded inside the blockchain which does the locking for it. However if the header chain is standalone, you cannot assume that a blockchain struct will protect it from hurting itself. Or that the client of the header chain will ensure not to do racey calls.
  • Along the same lines, passing sync objects through the constructor is similarly a bad idea, as it delegates synchronization to the client, making it extremely easy to misuse.
}

// NewBlockValidator returns a new block validator which is safe for re-use
func NewHdrValidator(chain *HeaderChain, pow pow.PoW) *HdrValidator {

This comment has been minimized.

@karalabe

karalabe Feb 4, 2016

Member

Please update the comment here. Also could we figure out a more decent name for this structr? Hdr doesn't cut it in a modern codebase :P

This comment has been minimized.

@fjl

fjl Feb 4, 2016

Contributor
                                                                                  It can be called headerValidator
@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Feb 4, 2016

@karalabe The basic idea here was that the header chain should NEVER be used alone, only inside a BlockChain or a LightChain (see #2120 ). This answers why the locking is done like this and also why there are no separate tests (LightChain is properly tested).
If there are some serious drawbacks of this design (not sure yet), I could think of other ways to share the pieces of code that I believe should be shared, I don't stick to my design ideas. Although I would have been nice to have this discussion six weeks ago :)

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Feb 4, 2016

I can accept that decision too, but then make the header chain private so that it doesn't leak out of the core package.

@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Feb 4, 2016

@karalabe the problem with that is that the LightChain is using it, and LightChain shouldn't be moved into the core package because it is light client stuff, it also uses the ODR interface.

@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Feb 4, 2016

Maybe instead of passing mutexes and semaphores (which looks a bit ugly, I can agree with that) the constructor could require an interface that is implemented by the parent chain (BlockChain or LightChain). So it is clear that this structure requires a parent to work. Would you like that better?

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Feb 4, 2016

But why is it so hard to make the thing thread safe on itself? :P

@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Feb 4, 2016

Not so hard, just completely pointless and a waste of resources :) A lock is always needed at the parent chain level.

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Feb 4, 2016

No, the waste of resources is the time spent debugging hidden dependencies caused by using something "wrong". Exposing something into the public API but requiring it to be used in a very specific way locking wise is asking for trouble and I guarantee it will break very soon.

It also adds a dependency from core to light, as we can no longer change the internals of the blockchain or the headerchain because the locking mechanisms are leaked out all over the rest of the code base and we have no guarantee that a minor modification will still be compatible with calling client code or not.

As long as you stay within the boundaries of a package, having access to internal mechanisms is completely fine, although I would argue that even then it's very ill advised. Providing access to package internals from outside of the package however is not something that should ever be done.

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Feb 4, 2016

Also just a heads up, this PR will conflict with the homestead PR on the selfish mining commit.

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-chain branch Feb 4, 2016

@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Feb 4, 2016

@karalabe issues are fixed

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-chain branch Feb 19, 2016

@karalabe karalabe added this to the 1.5.0 milestone Feb 19, 2016

@fjl

This comment has been minimized.

Copy link
Contributor

fjl commented Feb 23, 2016

👍

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-chain branch to 4908bf6 Feb 25, 2016

@@ -222,13 +219,10 @@ func (bc *BlockChain) SetHead(head uint64) {
bc.mu.Lock()
defer bc.mu.Unlock()

bc.hc.SetHead(head)

This comment has been minimized.

@karalabe

karalabe Mar 2, 2016

Member

This call here quite significantly alters the behavior of the original code. Notably it rewinds (i.e. deletes) the entire header chain before even touching anything else. The effect is, that the below code when trying to retrieve parent blocks to rewind, won't be able to because their headers are already missing, resulting in setting both the current block and the fast block to genesis.

The existing code first gathered all the hashes that it needed to dump and only after that began removing them. I think this is the same scenario as with rollback, that you need to iteratively do it for headers and blocks and cannot do one chain first and then the second.

Also note that below the code gathered the largest height of headers/blocks/fast-blocks and removed the canonical chain from that point. Your updated code only does this for headers, and the block/fast block heights are simply ignored (lines 225-235 are essentially nop). This part might actually not be a problem as the header chain can never be behind the block/fast-block chain, so it should be fine, but please delete this concept of max height calculation.

This comment has been minimized.

@karalabe

karalabe Mar 2, 2016

Member

There may be a very short and elegant solution to this, lets discuss after the standup.

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-chain branch Mar 2, 2016

@karalabe

View changes

core/blockchain.go Outdated
}
if bc.hc.CurrentHeader().Number.Uint64() < bc.currentFastBlock.NumberU64() {
bc.currentFastBlock = bc.GetBlock(bc.hc.CurrentHeader().Hash())
}

This comment has been minimized.

@karalabe

karalabe Mar 4, 2016

Member

Please move these two ifs to before the nil checks. As you are only modifying the currentBlock and currentFastBlock here, they cannot be nil before, however they can become nil with these assignments since the header may be ahead of the blocks, in case both will be nil after a SetHead.

This comment has been minimized.

@zsfelfoldi

zsfelfoldi Mar 4, 2016

Contributor

Moved up. Actually currentBlock can be nil before (at least in some tests), but now I'm checking for that too.

@karalabe

View changes

core/blockchain_test.go Outdated
bc := &BlockChain{chainDb: db, genesisBlock: genesis, eventMux: &eventMux, pow: FakePow{}}
bc.hc = &HeaderChain{chainDb: db, genesisHeader: genesis.Header(), rand: rand.New(rand.NewSource(0))}
bc.hc.getValidator = func() HeaderValidator { return bc.Validator() }
bc.hc.procInterrupt = bc.getProcInterrupt

This comment has been minimized.

@karalabe

karalabe Mar 4, 2016

Member

Please refactor the bc initialization to multiple lines and actually insert hc in there directly instead of an extra init step.

E.g.:

bc := &BlockChain{
  chainDb: db,
  genesisBlock: genesis,
  ...
  hc: &HeaderChain{
    ...
  },
}

This comment has been minimized.

@karalabe

karalabe Mar 4, 2016

Member

Actually, why aren't you using NewHeaderChain directly?

This comment has been minimized.

@zsfelfoldi

zsfelfoldi Mar 4, 2016

Contributor

I'm using NewHeaderChain directly now, but I can't insert it into bc init because for hc init we need a validator function which references bc.

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Mar 4, 2016

Small nitpick that please fix: use Callback not CallBack as type names (https://en.wikipedia.org/wiki/Callback_(computer_programming))

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-chain branch Mar 4, 2016

@karalabe karalabe modified the milestones: 1.4.0, 1.5.0 Mar 7, 2016

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Mar 7, 2016

Ah dammit, sorry, just saw: in the HeaderChain file you used both self as well as bc as the receiver type for methods. Please rewrite both to hc. Otherwise LGTM.

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-chain branch to 14dec34 Mar 7, 2016

@zsfelfoldi

This comment has been minimized.

Copy link
Contributor

zsfelfoldi commented Mar 7, 2016

@karalabe fixed :)

@karalabe

This comment has been minimized.

Copy link
Member

karalabe commented Mar 7, 2016

LGTM 👍

@zsfelfoldi zsfelfoldi force-pushed the zsfelfoldi:light-chain branch from 14dec34 to 73d21ea Mar 10, 2016

obscuren added a commit that referenced this pull request Mar 10, 2016

Merge pull request #2081 from zsfelfoldi/light-chain
core: create a header chain structure shared by full and light clients

@obscuren obscuren merged commit 8b58cd0 into ethereum:develop Mar 10, 2016

5 checks passed

buildbot/ARM Go pull requests DEV build done.
Details
buildbot/Linux Go pull requests DEV build done.
Details
buildbot/OSX Go pull requests DEV build done.
Details
buildbot/Windows Go pull requests DEV build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@obscuren obscuren removed the pr:review label Mar 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment