WIP: Taking finalization states into account when deciding to switch to the new chain #170

frolosofsky · 2018-10-15T16:22:00Z

It's a clean version of previous PR #164 after refactoring and fixing style comments.

Rough implementation of applying finalization to chain reorganization. It's WIP and in fact just a result of my initial unite/bitcoin sources research.

Some bullets/todo:

Need to explain new rule in an issue/ADR.
Because we started to reject parallel chains from older justified epochs, some of bitcoin's functional tests fail, e.g. we have 10 blocks in the main chain and 5th has been justified: now we reject all the blocks from parallel chains which height is less or equal 5. Discussed with @Gnappuraz, working around.
Tests
It's might be a good optimization to store FinalizationState per chain or per checkpoint instead of per block.

src/validation.cpp

Gnappuraz · 2018-10-16T08:52:19Z

src/esperanza/finalizationstate.h

@@ -216,6 +218,13 @@ class FinalizationState : public FinalizationStateData {

  mutable CCriticalSection cs_esperanza;

+  struct Storage {
+    std::map<uint256, FinalizationState> states;


A problem I see with this is that you never delete stuff from the map. Ideally whenever we reach finalization we can drop everything that comes before. This should behave more like a rolling window.

Good point.

Gnappuraz · 2018-10-16T08:52:26Z

src/esperanza/finalizationstate.cpp

+  auto const hash = index->GetBlockHash();
+  auto it = states.find(hash);
+  if (it == states.end()) {
+    it = states.emplace(hash, FinalizationState(*FindOrCreate(index->pprev)))


I don't think recursion is a good idea here, if someone serves you a very long header chain and then sends you the last block you would create a bazillion of intermediate states (easy DoS).

And again due my wrong assumption, I was thinking that recursion call MUST return in one hop because we already have state for previous tip of the chain new block belongs to.

This might become a correct solution if we manage to handle the block reconstruction so that we can assure consequentiality and ordering of stored blocks.

Gnappuraz · 2018-10-16T08:53:09Z

src/esperanza/finalizationstate.cpp

+FinalizationState *FinalizationState::Storage::FindOrCreate(
+    const CBlockIndex *index) {
+  LOCK(cs_storage);
+  if (index == nullptr || index->pprev == nullptr ||


if for some reason the block passed does not have a pointer to the pprev you are going to return the current main chain head state. Why?

It's not current main chain. esperanzaState.get() returns state for the genesis block. I thought that empty pprev means that it's block is genesis.

Yeah this you have to check thoroughly cause I wouldn't assume that there is no chance of a pIndex having pprev nullptr because of the previous block not being arrived yet.

Gnappuraz · 2018-10-16T09:37:27Z

src/validation.cpp

+
+    if (n->GetLastJustifiedEpoch() < c->GetLastJustifiedEpoch()) {
+        // UNIT-E TODO delete from the disk? If not, skip this condition.
+        LogPrintf("Reject outdated justified epoch: %d %d\n", n->GetLastJustifiedEpoch(), c->GetLastJustifiedEpoch());


As we discussed we don't reject but we keep it in case it becomes justified at a longer height.

Gnappuraz · 2018-10-16T09:39:34Z

src/validation.cpp

+    }
+
+    // Check whether new block's chain has forked after current's chain last justified block
+    uint256 checkpoint = c->GetCheckpointHash(c->GetLastJustifiedEpoch());


This should be GetLastFinalizedEpoch() and in general this check should be done when we receive the header since we don't need the full block to verify that.

I guess you meant GetLastFinalizedEpoch?

Yeah sure, sorry for that! Fixed.

Gnappuraz · 2018-10-16T09:40:02Z

src/validation.cpp

+    // Check whether new block's chain has forked after current's chain last justified block
+    uint256 checkpoint = c->GetCheckpointHash(c->GetLastJustifiedEpoch());
+    if (!checkpoint.IsNull() && !IsForkedAfter(checkpoint, pindex)) {
+        // UNIT-E TODO delete from the disk?


If we reject before AcceptBlock() nothing we'll be saved in disk.

Gnappuraz · 2018-10-16T09:41:11Z

Is a very good point that we should document the fork choice rule in a ADR, this would
also help implementation and review cause people have a reference where to get information
about this whole topic.

There are 4 main points for the fork-choice rule from casper (https://arxiv.org/pdf/1710.09437.pdf par.4):

never reorg before the last finalized checkpoint
build on top (follow) of the longest justified checkpoint
In case of no justification and forking within the same dynasty we fallback in order to
haviest chain (more stakes)
longest chain

Furthermore blocks that have forking before our last justification should be rejected straight away.
When we receive the header of a block we can already eagerly discard it in this way.
I think this will save a great deal of disk space compared to the current bitcoin behaviour.

scravy

I think a feature like this

needs to be thoroughly tested
should be documented/discussed in an ADR first

src/validation.cpp

Signed-off-by: Stanislav Frolov <[email protected]>

New code was pushed, I am dismissing my old request changes review.

scravy · 2018-11-02T00:12:08Z

src/net_processing.cpp

        }
+        return error("invalid header received");


I do not think that this move is correct. Previously: Would return this error in case of potential Denial-of-Service. Now with this change: Would return error in that case and if ProcessNewBlockHeaders/AcceptBlockHeader fails (for other reviewers: DoS is only checked then, hence the move one layer of if-conditions up – look at the context).

AFAICS: The AcceptBlockHeader looks at a single header. ProcessNewBlockHeaders looks at several headers. If it returns false then it did not validate some header, but there are also some valid headers which should be processed further down. Moving the return here will preempt that.

I had too many attempts to implement fork-choice, it might be an accidental artifact from previous one. Will check, thanks for this catch!

scravy · 2018-11-02T00:20:19Z

src/validation.cpp

@@ -2537,8 +2570,32 @@ CBlockIndex* CChainState::FindMostWorkChain() {
    do {
        CBlockIndex *pindexNew = nullptr;

+        if (chainActive.Tip() != nullptr) {


I remember that in some pull request I was asked whether the comparison withnullptr is necessary and a reference to the cpp core guidelines was given. I personally do not give a rats arse about this. But it popped into my mind – in that other pull request I obeyed the cpp core guidelines and removed the comparison with nullptr actually...

I remember that conversation. I'm not trying to go against the tide actually but it's just a strong habit. Previously I mentioned implicit type casting, but moreover this way is much more clear to understand I think. And actually cpp guidelines are bit inconsistent in this, e.g. at first they told:

void f(int i) { if (i) // suspect // ... if (i == success) // possibly better // ... }

And then...

// These all mean "if `p` is `nullptr`" if (!p) { ... } // good if (p == 0) { ... } // redundant `== 0`; bad: don't use `0` for pointers if (p == nullptr) { ... } // redundant `== nullptr`, not recommended

So in first example it's not redundant, but in seconds it's it. I don't see any logic in this. For sure I can review and change all such checks, it's not something I gonna fight for. But better I'd like to have discussion and consensus in the team and if we follow this guideline, let's put it in documentation. My point is to not follow guideline just because it's a guideline, but to understand why do we do so.

Actually I do like the explicit versions == nullptr and != nullptr better. But above all I value consistency :D

Anyways. Its a very very small nit. @Ruteri If you sent a pull request referencing the cpp core guidelines in developer notes the discussion could be taken there ;-)

frolosofsky · 2018-11-02T00:55:03Z

@scravy It's not finished yet, I still need to add bunch of tests at least, so I guess your previous change request remains valid.

Signed-off-by: Stanislav Frolov <[email protected]>

frolosofsky · 2018-11-07T15:13:26Z

I'm pausing development on this PR because it's blocked by #219.

mergeable · 2018-11-29T12:44:27Z