wallet: Minimal fix to restore conflicted transaction notifications
ClosedPublic
Actions

Authored by PiRK on Aug 27 2021, 10:20.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC250a0d5b2569: wallet: Minimal fix to restore conflicted transaction notifications

Summary

This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in
https://github.com/bitcoin/bitcoin/pull/18600.

Unlike that PR, which implements some new behavior, this just restores previous
wallet notification and status behavior for transactions removed from the
mempool because they conflict with transactions in a block. The behavior was
accidentally changed in two CWallet::BlockConnected updates:
a31be09bfd77eed497a8e251d31358e16e2f2eb1 and
7e89994133725125dddbfa8d45484e3b9ed51c6e from
https://github.com/bitcoin/bitcoin/pull/16624, causing issue
https://github.com/bitcoin/bitcoin/issues/18325.

The change here could be improved and replaced with a more comprehensive
cleanup, so it includes a detailed comment explaining future considerations.

Co-authored-by: Antoine Riard <ariard@student.42.fr>

This is a backport of core#18982 [1/2]
https://github.com/bitcoin/bitcoin/pull/18982/commits/b604c5c8b5892842f13dee89ae31812a28ab25d1

Backport note: The test is already included in D9972, because it seemed to work without this fix. But now we have seen intermittent CI failures.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Aug 27 2021, 10:20

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 27 2021, 10:20

PiRK requested review of this revision.Aug 27 2021, 10:20

teamcity edited the summary of this revision. (Show Details)Aug 27 2021, 10:20

PiRK edited the summary of this revision. (Show Details)Aug 27 2021, 10:21

PiRK added a child revision: D9963: trivial: Suggested cleanups to surrounding code.Aug 27 2021, 10:23

PiRK edited the summary of this revision. (Show Details)Aug 27 2021, 10:23

Harbormaster completed remote builds in B16441: Diff 29610.Aug 27 2021, 10:39

shouldn't the test for conflicted txns be on D9961?

In D9962#224992, @majcosta wrote:

shouldn't the test for conflicted txns be on D9961?

AFAIK, in D9961 the test wouldn't work for us. Core gets a notification for the bumpfee tx, but not for the conflict itself.

From https://github.com/bitcoin/bitcoin/pull/18878/files:

Screenshot from 2021-08-28 08-43-25.png (277×774 px, 70 KB)

On second thought, you are right that the code for generating the conflicting could be added in D9961 anyway. It would indeed be a bit closer to the original PRs.

PiRK mentioned this in D9961: test: Add test for tx notifications.Aug 28 2021, 07:04

PiRK mentioned this in D9972: test: Add test for conflicted tx notifications.Aug 30 2021, 07:03

rebase onto D9972 (removes the need to adjust feature_notifications.py at all). Remove the release note, because it seems the notification functionality for conflicting transactions was never broken in the first place.