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.

Sign up for GitHub

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

Jump to bottom

wallet_resendwallettransaction.py: fix coinbase height #16292

Closed
wants to merge 1 commit into from
Closed

wallet_resendwallettransaction.py: fix coinbase height #16292

wants to merge 1 commit into from

Conversation

instagibbs
Copy link
Member

Coinbase height is yet to be enforced on regtest, which allows these little errors in.

@DrahtBot DrahtBot added the Tests label Jun 26, 2019
@promag
Copy link
Contributor

promag commented Jun 27, 2019

From BIP34:

Height is the height of the mined block in the block chain, where the genesis block is height zero

So I think current code is correct?

@instagibbs
Copy link
Member Author

@promag you're making the next block, so no it's not. FWIW We have bip34 active on Elements functional tests which is how I ran into this.

@promag
Copy link
Contributor

promag commented Jun 27, 2019

@instagibbs I've just thought of this case: if getblockchaininfo()['blocks'] == 0 (no blocks) then genesis block must have height == 0.

@instagibbs
Copy link
Member Author

(assuming it's the same result as getblockcount) with genesis block already known, the value is 0.

it's literally just return chainActive.Height();

@promag
Copy link
Contributor

promag commented Jun 27, 2019

Actually node.getblockchaininfo()['blocks'] or node.getblockcount() return the chain height (it doesn't count genesis block). Sorry for the noise.

ACK eb3c8de.

@maflcko maflcko mentioned this pull request Jul 2, 2019
Merged
@promag
Copy link
Contributor

promag commented Jul 2, 2019

@instagibbs prefer #16292?

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 2, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16325 (rpc: Clarify that block count means height excl genesis by MarcoFalke)

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.

@instagibbs
Copy link
Member Author

closing in favor of #16325

@instagibbs instagibbs closed this Jul 2, 2019
@maflcko
Copy link
Member

maflcko commented Jul 2, 2019

Thanks, I am also working on activating BIP34 earlier in regtest ⌛

@instagibbs
Copy link
Member Author

I'm ~certain someone wrote a PR to do that already, but I cannot find it.

laanwj added a commit that referenced this pull request Jul 3, 2019
@laanwj
Merge #16325: rpc: Clarify that block count means height excl genesis
11de669 
fab0c82 rpc: Clarify that block count means height excl genesis (MarcoFalke)

Pull request description:

  There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in #16292 (comment).

  However, it really returns the height, which is `0` for the genesis block.

  So clarify that and also remove the misleading "longest blockchain" comment.

  Finally, fix the wallet test that incorrectly used this rpc.

ACKs for top commit:
  instagibbs:
    utACK fab0c82
  promag:
    ACK fab0c82, sorry for the misconception.

Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
monstrobishi pushed a commit to DeFiCh/ain that referenced this pull request Sep 6, 2020
@laanwj
Merge #16325: rpc: Clarify that block count means height excl genesis
afdbbb9 
fab0c82 rpc: Clarify that block count means height excl genesis (MarcoFalke)

Pull request description:

  There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in bitcoin/bitcoin#16292 (comment).

  However, it really returns the height, which is `0` for the genesis block.

  So clarify that and also remove the misleading "longest blockchain" comment.

  Finally, fix the wallet test that incorrectly used this rpc.

ACKs for top commit:
  instagibbs:
    utACK bitcoin/bitcoin@fab0c82
  promag:
    ACK fab0c82, sorry for the misconception.

Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Nov 4, 2021
@laanwj @Munkybooty
Merge bitcoin#16325: rpc: Clarify that block count means height excl …
01b553b 
…genesis

fab0c82 rpc: Clarify that block count means height excl genesis (MarcoFalke)

Pull request description:

  There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in bitcoin#16292 (comment).

  However, it really returns the height, which is `0` for the genesis block.

  So clarify that and also remove the misleading "longest blockchain" comment.

  Finally, fix the wallet test that incorrectly used this rpc.

ACKs for top commit:
  instagibbs:
    utACK bitcoin@fab0c82
  promag:
    ACK fab0c82, sorry for the misconception.

Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Nov 4, 2021
@laanwj @Munkybooty
Merge bitcoin#16325: rpc: Clarify that block count means height excl …
9363a84 
…genesis

fab0c82 rpc: Clarify that block count means height excl genesis (MarcoFalke)

Pull request description:

  There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in bitcoin#16292 (comment).

  However, it really returns the height, which is `0` for the genesis block.

  So clarify that and also remove the misleading "longest blockchain" comment.

  Finally, fix the wallet test that incorrectly used this rpc.

ACKs for top commit:
  instagibbs:
    utACK bitcoin@fab0c82
  promag:
    ACK fab0c82, sorry for the misconception.

Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Dec 7, 2021
@laanwj @vijaydasmp
Merge bitcoin#16325: rpc: Clarify that block count means height excl …
848d552 
…genesis

fab0c82 rpc: Clarify that block count means height excl genesis (MarcoFalke)

Pull request description:

  There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in bitcoin#16292 (comment).

  However, it really returns the height, which is `0` for the genesis block.

  So clarify that and also remove the misleading "longest blockchain" comment.

  Finally, fix the wallet test that incorrectly used this rpc.

ACKs for top commit:
  instagibbs:
    utACK bitcoin@fab0c82
  promag:
    ACK fab0c82, sorry for the misconception.

Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Dec 11, 2021
@laanwj @vijaydasmp
Merge bitcoin#16325: rpc: Clarify that block count means height excl …
5e75934 
…genesis

fab0c82 rpc: Clarify that block count means height excl genesis (MarcoFalke)

Pull request description:

  There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in bitcoin#16292 (comment).

  However, it really returns the height, which is `0` for the genesis block.

  So clarify that and also remove the misleading "longest blockchain" comment.

  Finally, fix the wallet test that incorrectly used this rpc.

ACKs for top commit:
  instagibbs:
    utACK bitcoin@fab0c82
  promag:
    ACK fab0c82, sorry for the misconception.

Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Dec 11, 2021
@laanwj @vijaydasmp
Merge bitcoin#16325: rpc: Clarify that block count means height excl …
1ec2d63 
…genesis

fab0c82 rpc: Clarify that block count means height excl genesis (MarcoFalke)

Pull request description:

  There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in bitcoin#16292 (comment).

  However, it really returns the height, which is `0` for the genesis block.

  So clarify that and also remove the misleading "longest blockchain" comment.

  Finally, fix the wallet test that incorrectly used this rpc.

ACKs for top commit:
  instagibbs:
    utACK bitcoin@fab0c82
  promag:
    ACK fab0c82, sorry for the misconception.

Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Dec 13, 2021
@laanwj @vijaydasmp
Merge bitcoin#16325: rpc: Clarify that block count means height excl …
aca95b4 
…genesis

fab0c82 rpc: Clarify that block count means height excl genesis (MarcoFalke)

Pull request description:

  There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in bitcoin#16292 (comment).

  However, it really returns the height, which is `0` for the genesis block.

  So clarify that and also remove the misleading "longest blockchain" comment.

  Finally, fix the wallet test that incorrectly used this rpc.

ACKs for top commit:
  instagibbs:
    utACK bitcoin@fab0c82
  promag:
    ACK fab0c82, sorry for the misconception.

Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Dec 14, 2021
@laanwj @vijaydasmp
Merge bitcoin#16325: rpc: Clarify that block count means height excl …
33cd5f5 
…genesis

fab0c82 rpc: Clarify that block count means height excl genesis (MarcoFalke)

Pull request description:

  There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in bitcoin#16292 (comment).

  However, it really returns the height, which is `0` for the genesis block.

  So clarify that and also remove the misleading "longest blockchain" comment.

  Finally, fix the wallet test that incorrectly used this rpc.

ACKs for top commit:
  instagibbs:
    utACK bitcoin@fab0c82
  promag:
    ACK fab0c82, sorry for the misconception.

Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Dec 15, 2021
@laanwj @vijaydasmp
Merge bitcoin#16325: rpc: Clarify that block count means height excl …
d55180d 
…genesis

fab0c82 rpc: Clarify that block count means height excl genesis (MarcoFalke)

Pull request description:

  There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in bitcoin#16292 (comment).

  However, it really returns the height, which is `0` for the genesis block.

  So clarify that and also remove the misleading "longest blockchain" comment.

  Finally, fix the wallet test that incorrectly used this rpc.

ACKs for top commit:
  instagibbs:
    utACK bitcoin@fab0c82
  promag:
    ACK fab0c82, sorry for the misconception.

Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Dec 15, 2021
@laanwj @vijaydasmp
Merge bitcoin#16325: rpc: Clarify that block count means height excl …
867371b 
…genesis

fab0c82 rpc: Clarify that block count means height excl genesis (MarcoFalke)

Pull request description:

  There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in bitcoin#16292 (comment).

  However, it really returns the height, which is `0` for the genesis block.

  So clarify that and also remove the misleading "longest blockchain" comment.

  Finally, fix the wallet test that incorrectly used this rpc.

ACKs for top commit:
  instagibbs:
    utACK bitcoin@fab0c82
  promag:
    ACK fab0c82, sorry for the misconception.

Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@instagibbs @promag @DrahtBot @maflcko