Skip to content

Conversation

@ryanofsky
Copy link
Contributor

Add test coverage for conflicted wallet transaction notifications so we can improve current behavior and avoid future regressions

#9240 - accidental break
#9479 - bug report
#9371 - fix
#16624 - accidental break
#18325 - bug report
#18600 - potential fix

Add test coverage for conflicted wallet transaction notifications so we improve
current behavior and avoid future regressions

bitcoin#9240 - accidental break
bitcoin#9479 - bug report
bitcoin#9371 - fix
bitcoin#16624 - accidental break
bitcoin#18325 - bug report
bitcoin#18600 - potential fix
@fanquake fanquake added the Tests label May 5, 2020
@DrahtBot
Copy link
Contributor

DrahtBot commented May 5, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

Nice! ACK

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

ACK 2c1c162

@ryanofsky
Copy link
Contributor Author

Thanks for reviews, updated 2c1c162 -> f963a68 (pr/nonot.2 -> pr/nonot.3, compare) with suggestions

@fanquake fanquake requested a review from maflcko May 13, 2020 10:47
@laanwj
Copy link
Member

laanwj commented May 13, 2020

Good to have a test for this, it always used to be somewhat ill-defined functionality, apparently breaking many times (reading that list hurts!).

ACK f963a68

@jonatack
Copy link
Member

re-ACK f963a68

@maflcko
Copy link
Member

maflcko commented May 13, 2020

ACK f963a68

@laanwj laanwj merged commit 99c0372 into bitcoin:master May 13, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 14, 2020
…tions

f963a68 test: Add test for conflicted wallet tx notifications (Russell Yanofsky)

Pull request description:

  Add test coverage for conflicted wallet transaction notifications so we can improve current behavior and avoid future regressions

  bitcoin#9240 - accidental break
  bitcoin#9479 - bug report
  bitcoin#9371 - fix
  bitcoin#16624 - accidental break
  bitcoin#18325 - bug report
  bitcoin#18600 - potential fix

ACKs for top commit:
  laanwj:
    ACK f963a68
  jonatack:
    re-ACK f963a68
  MarcoFalke:
    ACK f963a68

Tree-SHA512: d3a7952a2d3dc2ff0800ef857575ea4ef9759c0917d58a7fc91e2db0ca3cc3baf0dd0cf9af61683f691e5fefb11afe8120bb5810c7037ed9ecedee385dd4aa07
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 14, 2020
Add test coverage for conflicted wallet transaction notifications so we improve
current behavior and avoid future regressions

bitcoin#9240 - accidental break
bitcoin#9479 - bug report
bitcoin#9371 - fix
bitcoin#16624 - accidental break
bitcoin#18325 - bug report
bitcoin#18600 - potential fix

Github-Pull: bitcoin#18878
Rebased-From: f963a68
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 15, 2020
Add test coverage for conflicted wallet transaction notifications so we improve
current behavior and avoid future regressions

bitcoin#9240 - accidental break
bitcoin#9479 - bug report
bitcoin#9371 - fix
bitcoin#16624 - accidental break
bitcoin#18325 - bug report
bitcoin#18600 - potential fix

Github-Pull: bitcoin#18878
Rebased-From: f963a68
maflcko pushed a commit that referenced this pull request May 15, 2020
245c862 test: disable script fuzz tests (fanquake)
9a8fb4c fuzz: Remove enumeration of expected deserialization exceptions in ProcessMessage(...) fuzzer (practicalswift)
6161c94 [net processing] Only send a getheaders for one block in an INV (John Newbery)
cf2a6e2 test: Remove const to work around compiler error on xenial (Wladimir J. van der Laan)
cc7d344 miner: Avoid stack-use-after-return in validationinterface (MarcoFalke)
37a6207 test: Add unregister_validation_interface_race test (MarcoFalke)
ff4dc20 gui: Fix manual coin control with multiple wallets loaded (João Barbosa)
ed0afe8 test: Add test for conflicted wallet tx notifications (Russell Yanofsky)
251e321 rpc: Relock wallet only if most recent callback (João Barbosa)
ca4dac4 rpc: Add mutex to guard deadlineTimers (João Barbosa)
a3fe458 [docs] Improve commenting in ProcessGetData() (John Newbery)
011532e [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar)
1e73d72 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar)
fb82173 [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar)
315ae14 gui: Fix itemWalletAddress leak when not tree mode (João Barbosa)

Pull request description:

  Backports the following PRs to the 0.20 branch:

  * #18578: gui: Fix leak in CoinControlDialog::updateView
  * #18808: [net processing] Drop unknown types in getdata
  * #18814: rpc: Relock wallet only if most recent callback
  * #18878: test: Add test for conflicted wallet tx notifications
  * #18894: gui: Fix manual coin control with multiple wallets loaded
  * #18742: miner: Avoid stack-use-after-return in validationinterface
  * #18962: net processing: Only send a getheaders for one block in an INV
  * #18975: test: Remove const to work around compiler error on xenial

ACKs for top commit:
  promag:
    Tested ACK 245c862 coin control with multiple wallets.
  laanwj:
    ACK 245c862
  MarcoFalke:
    ACK 245c862 solved the conflicts myself as a sanity check. Did not re-review 🍷

Tree-SHA512: 285e5a5fad5bbeba6032742c65dc68836e8eccfcceda9e69fec4ddd162a3f61679a96f9bbe3d434267835af67c21ac4c05accf6f63e827c2eb47203c6daafe31
backpacker69 referenced this pull request in peercoin/peercoin Mar 28, 2021
Add test coverage for conflicted wallet transaction notifications so we improve
current behavior and avoid future regressions

bitcoin/bitcoin#9240 - accidental break
bitcoin/bitcoin#9479 - bug report
bitcoin/bitcoin#9371 - fix
bitcoin/bitcoin#16624 - accidental break
bitcoin/bitcoin#18325 - bug report
bitcoin/bitcoin#18600 - potential fix

Github-Pull: #18878
Rebased-From: f963a68
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Aug 30, 2021
Summary:
Add a couple of test for wallet notifications.

Backport notes:
 - The original PR dealt with conflicting transactions due to RBF trickery, which is not possible on the eCash blockchain. So the test is different, we simply create conflicting transactions while the nodes are disconnected.
 - In the original PR, the test for conflicting transactions does not work correctly. We do not observe the same bug. There is a notification for the conflicting transaction, as expected.

This is a backport of [[bitcoin/bitcoin#18878 | core#18878]]

Test Plan: `ninja check-functional`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D9972
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
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.

6 participants