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.

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

GBT fails consistently after some time. CreateNewBlock: TestBlockValidity failed: (code 0) #50

Closed
flound1129 opened this issue Dec 23, 2015 · 49 comments

Comments

@flound1129
Copy link

commented Dec 23, 2015

$ namecoin-cli getblocktemplate
error code: -1
error message:
CreateNewBlock: TestBlockValidity failed: (code 0)

Getting lots of these in the debug log:
2015-12-23 06:35:18 CreateNewBlock(): total size 1246 txs: 1 fees: 500000 sigops 101
2015-12-23 06:35:18 ERROR: CheckNameTransaction: NAME_FIRSTUPDATE on an unexpired name
2015-12-23 06:35:18 ERROR: CheckInputs: tx invalid for Namecoin
2015-12-23 06:35:18 ERROR: ConnectBlock(): CheckInputs on fc56b9f2297a299cf59022317b50ba45ace9392932453eb027f3a77bf4c90bb1 failed with (code 0)

@domob1812

This comment has been minimized.

Copy link

commented Dec 23, 2015

This seems like a good indication of what the issue may be - thanks a lot. I have a clue now what to check and possibly fix - although I think that I may only have time to do that after Christmas.

If someone wants to help further, you can try to find a reproducible test case on regtest mode - that would be great, but I think I may be able to fix it even without (and come up with one myself).

@domob1812

This comment has been minimized.

Copy link

commented Dec 23, 2015

This was easier than I thought - testing a fix right now and will submit a pull request once this is done. In the mean time, let me briefly describe what the issue is: This arises due to a combination of two recent changes in Bitcoin.

One concerned the mempool logic, which now uses a "dummy coinview" for testing new transactions before accepting into the mempool. This requires to explicitly load all necessary UTXO information into the dummy cache beforehand and was (I guess) done to improve performance. This also requires to explicitly load information about the name database. Part of that was done when I merged, but I forgot one part - this allowed to get registrations of existing names (which are invalid) into the mempool if the RPC code is "patched".

These transactions are, of course, invalid in a block and cannot be mined. The second change, now, was a rewrite of block creation in the miner. Previously, all transactions were individually checked against the created block before adding them, so that these invalid transactions would simply not make it into a block. According to the rewrite, this check is now gone and only ad-hoc tests are done for each transaction - again in the name of performance, I guess. Also here, I added some checks for Namecoin when merging, but they were not sufficient for the invalid firstupdates (as they are not supposed to get into the mempool at all).

All in all, this leads to the effect that the miner might create invalid blocks if the mempool contains these invalid transactions. This is not a "network-wide" problem, as the blocks are recognised as invalid as they should. But this makes the miner fail, which is the issue here. The fix is easy, we simply have to fix the mempool logic to recognise the transactions in question here.

domob1812 added a commit to domob1812/namecore that referenced this issue Dec 23, 2015
The loading of data into the temporary cache used by the mempool logic
was insufficient, we also need information about existing names that a
tx tries to "re-register".  Fix this.

See namecoin#50 for more
information.
@domob1812

This comment has been minimized.

Copy link

commented Dec 23, 2015

See #51 and, of you can, please try if this fixes the issue for you.

@flound1129

This comment has been minimized.

Copy link
Author

commented Dec 26, 2015

Doesn't look like that PR is passing tests, is that a problem?

@JeremyRand

This comment has been minimized.

Copy link
Member

commented Dec 27, 2015

@flound1129 The OSX failure is due to an issue with upstream Bitcoin Core's dependency server, and only affects Travis CI (not end users). The two Linux failures are both on proxy_test.py; I have no idea what impact they would have on end users. Might be a good idea to wait for @domob1812 to look at the proxy_test.py errors before using it for something as mission-critical as mining, but it doesn't appear to be an error relating to mining or consensus code, so I'd guess that it's safe -- but Daniel's the expert on this.

@domob1812

This comment has been minimized.

Copy link

commented Dec 27, 2015

I've not yet fully investigated the recent failures, but I'm sure it is not related to this change. I also run my own "critical" tests for each commit (including the tests of all the name-specific features), so the code "should" work fine nevertheless.

domob1812 added a commit to domob1812/namecore that referenced this issue Dec 29, 2015
The loading of data into the temporary cache used by the mempool logic
was insufficient, we also need information about existing names that a
tx tries to "re-register".  Fix this.

See namecoin#50 for more
information.
domob1812 added a commit to domob1812/namecore that referenced this issue Jan 9, 2016
The loading of data into the temporary cache used by the mempool logic
was insufficient, we also need information about existing names that a
tx tries to "re-register".  Fix this.

See namecoin#50 for more
information.
domob1812 added a commit to domob1812/namecore that referenced this issue Jan 11, 2016
The loading of data into the temporary cache used by the mempool logic
was insufficient, we also need information about existing names that a
tx tries to "re-register".  Fix this.

See namecoin#50 for more
information.
domob1812 added a commit to domob1812/namecore that referenced this issue Jan 11, 2016
The loading of data into the temporary cache used by the mempool logic
was insufficient, we also need information about existing names that a
tx tries to "re-register".  Fix this.

See namecoin#50 for more
information.
@bitkevin

This comment has been minimized.

Copy link

commented Sep 8, 2016

hey, I just put merged mining in btcpool and I have met the same issue:

  • version: v0.13.0rc1
  • os: Ubuntu 16.04 LTS 64 bits
$ namecoin-cli getauxblock
error code: -1
error message:
CreateNewBlock: TestBlockValidity failed:  (code 0)
2016-09-08 11:36:58 CreateNewBlock(): total size 43124 txs: 58 fees: 46700000 sigops 860
2016-09-08 11:36:58 ERROR: CheckNameTransaction: NAME_NEW is not mature for FIRST_UPDATE
2016-09-08 11:36:58 ERROR: CheckInputs: tx invalid for Namecoin
2016-09-08 11:36:58 ERROR: ConnectBlock(): CheckInputs on d8980bdf31ca5ad42721c7385384b6184ea70845a18a3f161f00f0e223975102 failed with  (code 0)
2016-09-08 11:36:58 keypool return 2
@bitkevin

This comment has been minimized.

Copy link

commented Sep 8, 2016

I have to add this shell script to make sure namecoind is works fine. More details in this commit.

#! /bin/bash
#
# namecoind getauxblock watcher shell
#
# this crontab shell is try to fix issue:
#   https://github.com/namecoin/namecoin-core/issues/50#issuecomment-245571582
#
# @copyright btc.com
# @author Zhibiao Pan
# @since 2016-09
#
SROOT=$(cd $(dirname "$0"); pwd)
cd $SROOT

NAMECOIND_RPC="namecoin-cli "

SUCCESS=`$NAMECOIND_RPC getauxblock | grep "previousblockhash" | wc -l`

if [[ $SUCCESS -ne 1 ]]; then
  # stop namecoind, supervisor will start it again
  echo "got error, restart namecoind..."
  $NAMECOIND_RPC stop
fi
@daizisheng

This comment has been minimized.

Copy link

commented Apr 24, 2017

I met this problem too and your way works, thanks @bitkevin

@crackfoo

This comment has been minimized.

Copy link

commented May 26, 2017

I'm still getting this problem even on the 0.14 branch... I'm not sure what the cause would be... is it from improper calling from my pool? Is that possible???

@JeremyRand

This comment has been minimized.

Copy link
Member

commented Jun 5, 2017

@domob1812

This comment has been minimized.

Copy link

commented Jun 7, 2018

@bitkevin, @crackfoo, @daizisheng: Is this still happening?

@domob1812 domob1812 added the bug label Jun 7, 2018
@crackfoo

This comment has been minimized.

Copy link

commented Jun 7, 2018

yup, still seeing this now and then... unsure why. will find a block or two then get that.

@domob1812

This comment has been minimized.

Copy link

commented Jun 7, 2018

Thanks, let me know if you find information to reproduce this for debugging.

@YihaoPeng

This comment has been minimized.

Copy link

commented Jun 17, 2018

I met the issue today with the old version: bd72abb
When I upgrade the node to the lastest version 570a547, the issue still exists.

So I stopped the node, deleted mempool.dat, and restart it. Then the issue disappeared.

The screenshot below is the error message and debug.log that I upgraded to 570a547 and not clear the memory pool:

beary229492132

beary229584801

@domob1812

This comment has been minimized.

Copy link

commented Jun 19, 2018

@YihaoPeng, thanks!

This is interesting - the offending transaction 6a09b86047f70ae148b03f7fb761ec6caf65325e99dae368d9489c85a45435ce is a name_firstupdate, whose name_new is also unconfirmed (16ba9e81c0cb48510a2f587851e10eff4aecda4fa7019c3f9b2cf7784fa70a22). This is something you cannot do with the RPC interface, but you can of course do if you construct the transactions somehow else. Presumably the logic to prevent adding name_firstupdates for immature name_news to a new block is broken in this edge case - I will take a look.

(And while those transactions are in the mempool, I can indeed reproduce the issue on my node when calling getblocktemplate.)

@domob1812 domob1812 self-assigned this Jun 19, 2018
@crackfoo

This comment has been minimized.

Copy link

commented Jun 19, 2018

@JeremyRand

This comment has been minimized.

Copy link
Member

commented Jun 19, 2018

Maybe this bug is the explanation for most of the pools dropping out 2 days ago. #227

@crackfoo

This comment has been minimized.

Copy link

commented Jun 19, 2018

@JeremyRand

This comment has been minimized.

Copy link
Member

commented Jun 19, 2018

Is there any way using the cli to invalidate that tx?

I'm not sure. @domob1812 would probably know. (I'll look around and see if I can find the answer in case Domob doesn't get back to us quickly on it.)

@JeremyRand

This comment has been minimized.

Copy link
Member

commented Jun 19, 2018

@domob1812 right, I meant deprioritise the name_firstupdate and positively prioritise the name_new.

@domob1812

This comment has been minimized.

Copy link

commented Jun 19, 2018

A hack to work around the issue would be to mark the tx as invalid at some place in the code. For instance in src/names/main.cpp inside CheckNameTransaction: Just compare the tx hash and if it matches the name_firstupdate, return false. This could be a good work around until I can fix the bug tomorrow.

@JeremyRand

This comment has been minimized.

Copy link
Member

commented Jun 19, 2018

A hack to work around the issue would be to mark the tx as invalid at some place in the code. For instance in src/names/main.cpp inside CheckNameTransaction: Just compare the tx hash and if it matches the name_firstupdate, return false. This could be a good work around until I can fix the bug tomorrow.

@domob1812 hmm, would that affect the consensus rules?

@domob1812

This comment has been minimized.

Copy link

commented Jun 19, 2018

Ah yes, indeed, it would cause issues afterwards. (But could be used to just mine the name_new.) Perhaps src/policy/policy.cpp and then IsStandard is better.

@domob1812

This comment has been minimized.

Copy link

commented Jun 19, 2018

Or just validation.cpp's AcceptToMemoryPool.

@JeremyRand

This comment has been minimized.

Copy link
Member

commented Jun 19, 2018

Yeah, the standardness check and mempool should be safe, since they're not consensus-related. It's probably worth trying prioritisetransaction first though, since that doesn't need a recompile and there's less that can go wrong.

@domob1812

This comment has been minimized.

Copy link

commented Jun 19, 2018

Agreed. I'll go offline for today soon - but will (hopefully) fix the bug correctly tomorrow. In the mean time, try prioritisetransaction first and then hacking AcceptToMemoryPool. As soon as someone manages to get the transaction mined we will be good. (Unless someone creates another such tx pair as an attack - but also that will be fixed tomorrow.)

domob1812 added a commit to domob1812/namecore that referenced this issue Jun 19, 2018
This might fix namecoin#50 and
namecoin#227, but is completely
untested and hacky.

I'll redo the fix with proper tests later.
@domob1812

This comment has been minimized.

Copy link

commented Jun 19, 2018

Completely untested potential fix for the underlying issue: domob1812@bcb4f33.

I'll redo this tomorrow with proper testing and perhaps in a different way (after thinking about it), but it may (or may not) fix the issue for now if anyone wants to try.

@JeremyRand

This comment has been minimized.

Copy link
Member

commented Jun 19, 2018

Here's a working fix.

namecoin-cli prioritisetransaction 6a09b86047f70ae148b03f7fb761ec6caf65325e99dae368d9489c85a45435ce 0.0 -1000000

I just tested it on my node, and I confirm that it made getblocktemplate work without errors (whereas previously it would error when both of those transactions were in the mempool). CC: @crackfoo

Once the name_new transaction has been mined and has 12 confirmations, you'll probably need to undo the above step like this:

namecoin-cli prioritisetransaction 6a09b86047f70ae148b03f7fb761ec6caf65325e99dae368d9489c85a45435ce 0.0 1000000

Which will allow the name_firstupdate to be mined. (Deleting the mempool might also work for this, I'm not sure.)

Kudos to https://bchain.info/NMC/ for making unconfirmed transactions' raw hex easily available, which allowed me to quickly test this.

@JeremyRand

This comment has been minimized.

Copy link
Member

commented Jun 19, 2018

(Obviously this fix is a stopgap, and once a proper code fix is merged to Namecoin Core, this stopgap will be unnecessary.)

@YihaoPeng

This comment has been minimized.

Copy link

commented Jun 20, 2018

To fix the issue, simply don't adding the immature name_firstupdate when generating a new block (getblocktemplate of createauxblock). I think there is no need to avoiding it be added to the memory pool.

This is like a transaction with locktime, it can be added to the memory pool, but cannot enter the block immediately.

After reaching the height of the specified of locktime, the transaction is automatically added to the block. The user don't need to wait for this and re-broadcast the transaction.

It looks like domob1812@bcb4f33 is such a fix. I don't think this is a "hacky" but a reasonable fix.

I will try to deploy a node of domob1812@bcb4f33 for mining. 😄

@JeremyRand

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

@domob1812 IMO it's a bad idea for Namecoin Core to allow relaying of name_firstupdate transactions that are spending immature name_new outputs. Relaying such transactions allows any third party on the Namecoin network to trivially frontrun the name registration, since it reveals the name that's being registered. I strongly suspect that anyone who's creating and broadcasting such transactions is doing so accidentally. So unless there's a use case that I'm missing, my vote is that Namecoin Core should reject such transactions from the mempool and not relay them.

@domob1812

This comment has been minimized.

Copy link

commented Jun 20, 2018

One thing to note: As soon as the offending transaction gets mined (by one miner being successful with prioritisetransaction), the issue will be resolved. There will not be a need for any other miner to perform that "fix" as well.

@JeremyRand: I think it is perfectly fine to broadcast a name_firstupdate after the name_new has a few but not necessarily 12 confirmations - this just slightly weakens the security, as it reduces the number of confirmations you have in advance of anyone trying to front run. But 12 are super cautious and you might choose to be fine with less. The use would be not having to wait for 12 confirmations before you go offline, for instance, if you are fine with taking the small risk.

Of course, broadcasting a name_firstupdate with an unconfirmed name_new as was the case here is dangerous. But note that this is something that the current standard RPC interface doesn't allow already (somewhat by accident, but still). So I think as we are good as long as we fix the mining issue (which I will do now).

@YihaoPeng

This comment has been minimized.

Copy link

commented Jun 20, 2018

@JeremyRand Yes, you're right. This can indeed lead to cybersquatting. I did not realize this problem before.

BTC.COM are mining the offending transaction now.

domob1812 added a commit to domob1812/namecore that referenced this issue Jun 20, 2018
When creating blocks from the mempool, miners need to make sure that they
do not include name_firstupdate transactions with still immature name_new
inputs; those are allowed in the mempool but not in the blockchain (but
they will be fine once the name_new is mature enough).

There is already logic to ensure that, but it had a bug that did not
prevent name_firstupdate's to be included in blocks if the corresponding
name_new was actually *unconfirmed*.

This caused namecoin#50 and
presumably also namecoin#227.

With this commit, we fix the underlying bug and add regression tests
that exercise and cover this scenario (as well as general handling of
immature / unconfirmed inputs to name operations).
@domob1812

This comment has been minimized.

Copy link

commented Jun 20, 2018

The proposed fix is in #228. It is very similar to my "untested fix" from above, but now includes proper regtests (which did reproduce the original issue before applying the code fix). The "untested fix" also broke the use of unconfirmed currency inputs, for instance from multiple name_firstupdate's in a wallet with only one coin output. This is fixed (and verified in the regtest) now as well.

@JeremyRand

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

@crackfoo may I ask whether you're a solo miner or a pool operator? If you're a pool operator, may I ask which pool? (I'd like to credit the people and pools who were involved in helping us get the issue fixed.)

@crackfoo

This comment has been minimized.

Copy link

commented Jun 20, 2018

I'm a pool op (http://zpool.ca), and the workarounds don't last for long before the error comes up again but I've not tried the #228 fixed yet. Shall I try that now for testing?

@JeremyRand

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

@crackfoo yes, it would be awesome if you could test #228 . (That said, I'd expect the issue to be resolved anyway since the offending transactions are mined by now... unless there are other transactions in the mempool that also trigger the issue?)

@crackfoo

This comment has been minimized.

Copy link

commented Jun 20, 2018

@JeremyRand

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

@crackfoo any test results?

@crackfoo

This comment has been minimized.

Copy link

commented Jun 22, 2018

@crackfoo

This comment has been minimized.

Copy link

commented Jun 22, 2018

ok, got issues sorted.

Running now with that test code. Will let you know if the error pops up again.

domob1812 added a commit that referenced this issue Jun 25, 2018
8107e23 Correctly handle unconfirmed name_new in mining. (Daniel Kraft)

Pull request description:

  When creating blocks from the mempool, miners need to make sure that they do not include `name_firstupdate` transactions with still immature `name_new` inputs; those are allowed in the mempool but not in the blockchain (but they will be fine once the name_new is mature enough).

  There is already logic to ensure that, but it had a bug that did not prevent `name_firstupdate`'s to be included in blocks if the corresponding `name_new` was actually *unconfirmed*.

  This caused #50 and presumably also #227.

  With this commit, we fix the underlying bug and add regression tests that exercise and cover this scenario (as well as general handling of immature / unconfirmed inputs to name operations).

Tree-SHA512: 9aca6529ebff2ddcd1ba2457953df9e89037b7060a7b817cb368c21fea677528f51ee2acc2ad1d03c25b639cb981a815b4025733ed2bde931d193c69b5821276
@domob1812

This comment has been minimized.

Copy link

commented Jun 25, 2018

This should be fixed now with #228. Feel free to reopen if not, or to open new issue if different problems occur in the future.

@domob1812 domob1812 closed this Jun 25, 2018
domob1812 added a commit to domob1812/namecore that referenced this issue Jun 28, 2018
When creating blocks from the mempool, miners need to make sure that they
do not include name_firstupdate transactions with still immature name_new
inputs; those are allowed in the mempool but not in the blockchain (but
they will be fine once the name_new is mature enough).

There is already logic to ensure that, but it had a bug that did not
prevent name_firstupdate's to be included in blocks if the corresponding
name_new was actually *unconfirmed*.

This caused namecoin#50 and
presumably also namecoin#227.

With this commit, we fix the underlying bug and add regression tests
that exercise and cover this scenario (as well as general handling of
immature / unconfirmed inputs to name operations).

This is a backport of 8107e23
to the 0.16 branch.  The regtest has been adjusted to work with the
older codebase and not require some later changes to the RPC interface.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.