Use equivalent PoW for non-main-chain requests #5918

pull sipa wants to merge 1 commits into bitcoin:master from sipa:betterfinger changing 4 files +49 −4

sipa commented at 1:51 PM on March 17, 2015: member

In addition to checking whether the age of the requested block isn't too old, also check whether its cummulative proof of work is not more than 1 month behind on the best chain (at its current speed).

This should prevent a fingerprinting attack described by @sergiodemianlerner on #5820.
laanwj added the label P2P on Mar 18, 2015
gmaxwell commented at 10:06 PM on March 20, 2015: contributor

I'm not super enthusiastic of using work as a float here, but it seems correct and harmless.

utACK. (I don't currently have something setup to test this specific behavior.)
sipa force-pushed on Mar 23, 2015
sipa commented at 1:30 PM on March 23, 2015: member

Updated to use uint256 arithmetic.
sipa force-pushed on Mar 23, 2015
laanwj commented at 9:52 AM on March 24, 2015: member

utACK
sipa commented at 11:58 AM on March 24, 2015: member

This patch is running on bitcoin.sipa.be, if someone wants to test whether syncing works.
gmaxwell commented at 6:17 PM on April 1, 2015: contributor

ACK
sipa commented at 5:02 PM on April 8, 2015: member

@sergiodemianlerner Feel like commenting?
SergioDemianLerner commented at 8:44 PM on April 17, 2015: contributor

When *mi->second has greater nChainWork than *pindexBestHeader, then subtraction in GetBlockProofEquivalentTime() will overflow the uint256 and zero wrap. Multiplication by TargetSpacing() can also overflow the uint256. This will prevent the block from being served. Of course, mi->second should never have more chain work than the tip (if it does it would become the new tip). Nevertheless, as a protective layer against other bugs, I would add a condition that if mi->second has greater nChainWork than the tip, the block is always served.
gmaxwell commented at 9:24 PM on April 17, 2015: contributor

@SergioDemianLerner how about an assert? (if that case ever becomes possible, then something is horribly wrong; I think.)
gmaxwell commented at 10:28 PM on April 17, 2015: contributor

@SergioDemianLerner how about an assert? (if that case ever becomes possible, then something is horribly wrong; I think.)
sdaftuar commented at 3:46 PM on April 20, 2015: member

@gmaxwell I'm not sure an assert makes sense here; the way the code seems to be structured now is that we don't generally hold a lock on cs_main in between updating mapBlockIndex (after receiving a block) and calling ActivateBestChain, so accessing chainActive.Tip() in between could result in an assertion failure.

I can't think of how this could occur outside of an interaction with the invalidateblock rpc call, but for example if you look at the implementation for invalidateblock, it calls InvalidateBlock while holding cs_main, then releases the lock and calls ActivateBestChain (which will lock cs_main itself where it needs to). In between, it could be the case that some other VALID_SCRIPTS block has more work than chainActive.Tip().

Perhaps we should clarify precisely what the semantics ought to be for chainActive.Tip(): should it only be accessible if it reflects all known information about what the best chain should be?
sipa commented at 4:00 PM on April 20, 2015: member

Let's just fix the code to deal with negatives.
sipa force-pushed on Apr 21, 2015
sipa commented at 3:32 PM on April 21, 2015: member

Updated.
Use equivalent PoW for non-main-chain requests f7303f9793

in src/main.cpp:None in ae84ff31e4 outdated

3493 | +                        // chain if they are valid, and no more than a month older (both in time, and in
3494 | +                        // best equivalent proof of work) than the best header chain we know about.
3495 |                          send = mi->second->IsValid(BLOCK_VALID_SCRIPTS) && (pindexBestHeader != NULL) &&
3496 | -                            (mi->second->GetBlockTime() > pindexBestHeader->GetBlockTime() - 30 * 24 * 60 * 60);
3497 | +                            (mi->second->GetBlockTime() > pindexBestHeader->GetBlockTime() - nOneMonth) &&
3498 | +                            (GetBlockProofEquivalentTime(*mi->second, *pindexBestHeader, *pindexBestHeader) > nOneMonth);

morcos commented at 4:18 PM on April 21, 2015:

Is this supposed to be < nOneMonth?

sipa commented at 5:01 PM on April 21, 2015:

Ugh, yes. This is why code like this needs tests...

sipa force-pushed on Apr 22, 2015
sipa commented at 11:01 AM on April 22, 2015: member
- Rebased
- Changed the argument order of GetBlockProofEquivalentTime(a, b, c, d) to be identical to a->GetBlockTime () - b->GetBlockTime().
- Changed the order of calls in the fingerprinting test to be consistent with this.
- Passed Consensus::Params as an argument (make @jtimon happy).
- Added a simple unit test.
gavinandresen commented at 2:43 PM on April 27, 2015: contributor

ACK
sipa merged this on Apr 28, 2015
sipa closed this on Apr 28, 2015
sipa referenced this in commit 7bf5d5efa6 on Apr 28, 2015

in src/pow.cpp:None in f7303f9793

 123 | +        r = to.nChainWork - from.nChainWork;
 124 | +    } else {
 125 | +        r = from.nChainWork - to.nChainWork;
 126 | +        sign = -1;
 127 | +    }
 128 | +    r = r * arith_uint256(params.nPowTargetSpacing) / GetBlockProof(tip);

rebroad commented at 4:19 PM on December 23, 2016:

nPowTargetSpacing, which always equals 600 regardless.... why can't this just be a global variable? It would save having to pass around the params everywhere.

MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#5820 Better fingerprinting protection for non-main-chain getdatas.