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

index: Fix for indexers skipping genesis block. #14085

Merged
merged 1 commit into from Jan 9, 2019

Conversation

Projects
None yet
@jimpo
Copy link
Contributor

commented Aug 27, 2018

This fixes a bug where indexers would skip processing of the genesis block. Preserves the current behavior of omitting genesis block transaction from the index.

@instagibbs

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

@sipa has NACK'd this previously when I offered a "fix".

Reasoning is something like: Txindex was used previous to Core maintaining a pruned utxo set, and thus never had use for the genesis block transaction, since it was never entered into the utxo set.

I personally concept ACK, just giving background.

@jimpo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

Even if we decide to exclude the genesis transaction from the txindex specifically, we should still fix in the BaseIndex and add an exception in the TxIndex subclass.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

What was the previous behaviour (e.g. in version 0.15 with -txindex)?

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

That transaction isn't really in the blockchain in almost any meaningful sense: It has no effect on the system state. e.g. it shouldn't show up in address indexing either, because although it looks spendable it's not spendable.

@jimpo jimpo force-pushed the jimpo:txindex-genesis-fix branch to ed12d5d Aug 28, 2018

@jimpo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2018

@MarcoFalke I'm pretty sure prior behavior was to include it in the index. <- That's wrong, it was omitted from the index. I ran a 0.15.1 node to verify.

@sipa

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

The genesis transaction was never included in any index, intentionally.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Aug 30, 2018

IMO, a merkle root of 4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b indicates the block has 0 transactions. ;)

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

~0 on this

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14121 (Index for BIP 157 block filters by jimpo)
  • #14111 (index: Create IndexRunner class for activing indexes. by jimpo)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@sipa

This comment has been minimized.

Copy link
Member

commented Nov 10, 2018

~0 on this as well - it could be considered if there is ever an indexer that needs to observe the genesis block somehow, but I don't see why that would be the case.

@jimpo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2018

@sipa The block filter headers commit the the filter header of the previous block and BIP 157 defines a filter header for the genesis block, which is the motivation for this PR. The BIP could be amended to define the filter header of the genesis as 0, but I feel like this is a silly reason to change the BIP. Also, the genesis block is a block -- it's very unintuitive to me that it would get skipped by default just because it's a special block with no effect on the chain state.

@sipa

This comment has been minimized.

Copy link
Member

commented Nov 10, 2018

@jimpo I see, that's a perfectly good reason. There is no restriction on downloading the genesis block either.

@sipa

This comment has been minimized.

Copy link
Member

commented Nov 11, 2018

utACK ed12d5d

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Nov 11, 2018

utACK ed12d5d

@jamesob

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

utACK ed12d5d

@luke-jr

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

@jimpo The genesis block de facto has ZERO transactions. IMO it is a bug in BIP 157 if it considers that it does.

@jimpo

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2018

@luke-jr Per BIP 157, the genesis block does have a filter and it has a single element, an output. Since this output is unspendable, this does seem like a bug on reflection. Would everyone be happier if I change BIP 157 so that there is no filter for the genesis block and it's filter header becomes 0x00{32}?

@Roasbeef

@laanwj laanwj added this to Blockers in High-priority for review Jan 3, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

This has enough (ut)ACKs, but the last post makes it unclear to me whether to merge this now, probably better to wait for BIP clarification or not?

@Roasbeef

This comment has been minimized.

Copy link

commented Jan 3, 2019

@jimpo I don't see a good reason to modify the format of the filters this late in the cycle. A number of individuals are already running BIP 157/158 on mainnet as other full node implementations are already able to serve them. I think we all understand that the genesis block is unspendable, but it does in fact have transactions, though they don't modify the UTXO set. Why add another special case?

@jimpo

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2019

@laanwj I think the BIP is fine and I haven't heard serious objections. I think this should get merged and if there is a call to change the genesis filter in the BIP it's a one-line change (similar to what was done with txindex). I still think 100% that the base index should not skip the genesis block, which is the main thing this fixes.

@jamesob

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

This is ready for merge IMO.

@laanwj laanwj merged commit ed12d5d into bitcoin:master Jan 9, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 9, 2019

Merge #14085: index: Fix for indexers skipping genesis block.
ed12d5d index: Fix for indexers skipping genesis block. (Jim Posen)

Pull request description:

  This fixes a bug where indexers would skip processing of the genesis block. Preserves the current behavior of omitting genesis block transaction from the index.

Tree-SHA512: 092fd3d629bf1ef279566217c668cc913a8b8e012d811d0e544231894c49a0c0c179537ac4727c39b9bf407479541745d79c4e118db6f0795a2b848d0fe62cbf

@laanwj laanwj removed this from Blockers in High-priority for review Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.