Skip to content

Conversation

Bushstar
Copy link
Contributor

vTxHashes is a vector of all entries in mapTx, if you clear one you should clear the other, lest someone try to use the txiter in vTxHashes which would result in a segfault.

@glozow
Copy link
Member

glozow commented Jan 25, 2022

light code review ACK 2902780

Perhaps someone with more historical insight should look at this as well. I can't find a reason for vTxHashes to not be cleared. It looks like it was introduced this way in 8119026.

@maflcko
Copy link
Member

maflcko commented Jan 26, 2022

It might be better to remove the whole function instead. See #20030 and then #19909

@glozow
Copy link
Member

glozow commented Jan 26, 2022

It might be better to remove the whole function instead. See #20030 and then #19909

Oh, agree. Is #19909 up for grabs?

@maflcko
Copy link
Member

maflcko commented Jan 26, 2022

#19909 is the trivial part, #20030 is the tougher one to review even though it only changes on line. I can reopen it later today.

@luke-jr
Copy link
Member

luke-jr commented Jan 29, 2022

Regardless of removing the whole function later, this seems to be a good idea and should be reviewed/merged for backport benefit.

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

utACK, 1 nit

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

re-utACK

@laanwj
Copy link
Member

laanwj commented Apr 6, 2022

Code review ACK 9d65ad3
This makes sense for consistency. As long as there is a clear function it makes sense to clear everything and not forget this. Also agree on the long-run goal of removing it in favor of safer approaches.

@laanwj laanwj changed the title Clear vTxHashes when mapTx is cleared mempool: Clear vTxHashes when mapTx is cleared Apr 6, 2022
@laanwj laanwj merged commit bbb83f0 into bitcoin:master Apr 6, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 6, 2022
9d65ad3 Clear vTxHashes when mapTx is cleared (Peter Bushnell)

Pull request description:

  vTxHashes is a vector of all entries in mapTx, if you clear one you should clear the other, lest someone try to use the txiter in vTxHashes which would result in a segfault.

ACKs for top commit:
  laanwj:
    Code review ACK 9d65ad3

Tree-SHA512: 6832755e43ab7f528b46817aeadcb6ffdc965b97f59ab96bb053dedbb7e68155ba3db52286355dca33b509237f80eda249760b26db493762bc50d8e2cef16d8f
glozow added a commit to bitcoin-core/gui that referenced this pull request Jan 4, 2023
…r() helper

fa818e1 txmempool: Remove unused clear() member function (MarcoFalke)

Pull request description:

  Seems odd to have code in Bitcoin Core that is unused.

  Moreover the function was broken (see bitcoin/bitcoin#24145) and is brittle, as there is nothing that prevents similar bugs from re-appearing.

  Fix both issues by replacing it with C++11 member initializers.

ACKs for top commit:
  glozow:
    ACK fa818e1

Tree-SHA512: e79e44cac7d5a84d9ecc8e3f3b0b9a50e1e3ebec358b20ba5dac175ef07d1fbe338a20f83ee80f746f7c726c79e77f8be49e14bca57a41063da8a5302123c3a9
@bitcoin bitcoin locked and limited conversation to collaborators Apr 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants