Skip to content

Conversation

@jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Oct 9, 2018

Backport of PR #14411 to v0.17.

This change partially reverts #13075 and #14023.

Fixes #14382

@jnewbery jnewbery changed the title [wallet] Restore ability to list incoming transactions by label [wallet] Backport(0.17): Restore ability to list incoming transactions by label Oct 9, 2018
@fanquake fanquake added this to the 0.17.1 milestone Oct 9, 2018
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.

utACK c8d1f1513fb39d80cebbc32e1b5327209485e085

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be listtransactions with s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@ryanofsky ryanofsky Oct 10, 2018

Choose a reason for hiding this comment

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

This paragraph doesn't really make sense when both behaviors are present in the same release. I'd maybe change it to:

When bitcoin is configured with the -deprecatedrpc=accounts setting,
specifying a label/account/dummy argument will return both outgoing and
incoming transactions. Without the -deprecatedrpc=accounts setting, it will
only return incoming transactions (because it used to be possible to create
transactions spending from specific accounts, but this is no longer possible
with labels).

Copy link
Contributor

@ryanofsky ryanofsky Oct 10, 2018

Choose a reason for hiding this comment

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

I think it'd be good to document the empty string change. Maybe:

Also for backwards compatibility when -deprecatedrpc=accounts is set, it's
possible to pass an empty string label "" to list transactions that aren't
labeled. Without -deprecatedrpc=accounts, passing the empty string is an
error because returning only non-labeled transactions isn't actually very
useful, and can be confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both are good changes. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how the new check for "*" here changes anything. Maybe drop for clarity (unless I'm missing something).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, you're right. It's better without. The behaviour would be identical with or without the check for "*". I've removed it.

Backport of PR 14411 to v0.17.

This change partially reverts bitcoin#13075 and bitcoin#14023.

Fixes bitcoin#14382
@jnewbery jnewbery force-pushed the restore_listtransactions_label branch from c8d1f15 to 89306ab Compare October 10, 2018 07:35
@jnewbery
Copy link
Contributor Author

Thanks for the review @ryanofsky! I've updated the PR with your changes.

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.

utACK 89306ab. Only changes since last review were code simplification and release notes updates suggested above.

@promag
Copy link
Contributor

promag commented Oct 10, 2018

utACK 89306ab.

@meshcollider
Copy link
Contributor

meshcollider commented Nov 9, 2018

utACK 89306ab

This is slightly different from other backport PRs because the changes to src/wallet/rpcwallet.cpp are significantly different from the original PR to achieve the same functionality. The tests are basically a clean backport though.

@laanwj laanwj merged commit 89306ab into bitcoin:0.17 Nov 10, 2018
laanwj added a commit that referenced this pull request Nov 10, 2018
…ng transactions by label

89306ab [wallet] Restore ability to list incoming transactions by label (Russell Yanofsky)

Pull request description:

  Backport of PR #14411 to v0.17.

  This change partially reverts #13075 and #14023.

  Fixes #14382

Tree-SHA512: 1f8300e1a79e826cd706561265b8788deef505fa510be1a76ed9a62e5fca37cf6a741423ac0e5de2a36d6e8b9f25f141885455aacacbbf6474814e6eae406a27
@jnewbery jnewbery deleted the restore_listtransactions_label branch October 9, 2019 22:20
xdustinface pushed a commit to xdustinface/dash that referenced this pull request Dec 22, 2020
… incoming transactions by label

89306ab [wallet] Restore ability to list incoming transactions by label (Russell Yanofsky)

Pull request description:

  Backport of PR bitcoin#14411 to v0.17.

  This change partially reverts bitcoin#13075 and bitcoin#14023.

  Fixes bitcoin#14382

Tree-SHA512: 1f8300e1a79e826cd706561265b8788deef505fa510be1a76ed9a62e5fca37cf6a741423ac0e5de2a36d6e8b9f25f141885455aacacbbf6474814e6eae406a27
UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Aug 7, 2021
… to list incoming transactions by label"

This reverts commit 96d3063.
UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Sep 17, 2021
… to list incoming transactions by label"

This reverts commit 96d3063.
UdjinM6 added a commit to dashpay/dash that referenced this pull request Sep 24, 2021
@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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants