Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Oct 21, 2021

Follow-up to:

  • commit 700c42b, which replaced pIndex
    with block_hash in AddToWalletIfInvolvingMe.

  • commit 9700fcb, which replaced
    posInBlock with confirm.nIndex.

Follow-up to:
* commit 700c42b, which replaced pIndex
  with block_hash in AddToWalletIfInvolvingMe.

* commit 9700fcb, which replaced
  posInBlock with confirm.nIndex.
@maflcko maflcko force-pushed the 2110-docWalletConfirm branch from faf9a9f to fa8fef6 Compare October 21, 2021 20:14
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 22, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)

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.

@laanwj
Copy link
Member

laanwj commented Oct 22, 2021

Code review ACK fa8fef6

@maflcko
Copy link
Member Author

maflcko commented Oct 22, 2021

cc Ryan @ryanofsky

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK fa8fef6

This is probably ready for merge, but if anybody else reviews this they may also be interested in #21206 which changes the same code and does a broader cleanup.

Comment on lines -288 to -289
/* Used by TransactionAddedToMemorypool/BlockConnected/Disconnected/ScanForWalletTransactions.
* Should be called with non-zero block_hash and posInBlock if this is for a transaction that is included in a block. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "doc: Fix CWalletTx::Confirmation doc" (fa8fef6)

Minor: This comment does seem useful to me and I'd probably s/non-zero block_hash and posInBlock/confirmation/ instead of dropping it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already explained in the Confirmation doc:

/** Confirmation includes tx status and a triplet of {block height/block hash/tx index in block} ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think this is important to explain, it might be better to put the comment to TxStateConfirmed from #21206?

@maflcko maflcko merged commit af4275e into bitcoin:master Oct 26, 2021
@maflcko maflcko deleted the 2110-docWalletConfirm branch October 26, 2021 12:52
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 26, 2021
fa8fef6 doc: Fix CWalletTx::Confirmation doc (MarcoFalke)

Pull request description:

  Follow-up to:
  * commit 700c42b, which replaced pIndex
    with block_hash in AddToWalletIfInvolvingMe.

  * commit 9700fcb, which replaced
    posInBlock with confirm.nIndex.

ACKs for top commit:
  laanwj:
    Code review ACK fa8fef6
  ryanofsky:
    Code review ACK fa8fef6

Tree-SHA512: e7643b8e9200b8ebab3eda30435ad77006568a03476006013c9ac42c826c84e42e29bcdefc6f443fe4d55e76e903bfed5aa7a51f831665001c204634b6f06508
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants