New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RPC] Add import/removeprunedfunds rpc call #7558

Merged
merged 2 commits into from Mar 29, 2016

Conversation

Projects
None yet
6 participants
@instagibbs
Member

instagibbs commented Feb 19, 2016

This allows wallets to import funds without a rescan. It required an address or private key to exist in the wallet before calling. Primarily to be used to import funds to a pruned wallet, but could be used in conjunction with importaddress/privkey without rescan on an archival node.

A companion RPC call "removeprunedfunds" is added to allow the user to delete erroneous imported funds.

@instagibbs instagibbs changed the title from Add importprunedfunds rpc call to [RPC] Add importprunedfunds rpc call Feb 19, 2016

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Feb 19, 2016

Member

Concept ACK.

What happens if I import a fully spent transaction but fail to import the transaction(s) spending it's outputs?

Member

gmaxwell commented Feb 19, 2016

Concept ACK.

What happens if I import a fully spent transaction but fail to import the transaction(s) spending it's outputs?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 19, 2016

Member

What happens if I import a fully spent transaction but fail to import the transaction(s) spending it's outputs?

You are eaten by a grue.

Member

sipa commented Feb 19, 2016

What happens if I import a fully spent transaction but fail to import the transaction(s) spending it's outputs?

You are eaten by a grue.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Feb 19, 2016

Member

One could avoid the grue eating by using a lantern, I mean, checking if the output is still spendable and setting a flag... perhaps?

Member

gmaxwell commented Feb 19, 2016

One could avoid the grue eating by using a lantern, I mean, checking if the output is still spendable and setting a flag... perhaps?

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 19, 2016

Contributor

The build fails with this on some systems:

/bin/sh: 1: /home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/importprunedfunds.py: Permission denied
Contributor

paveljanik commented Feb 19, 2016

The build fails with this on some systems:

/bin/sh: 1: /home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/importprunedfunds.py: Permission denied
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 19, 2016

Member

Concept ACK, +1 for the grue

Member

laanwj commented Feb 19, 2016

Concept ACK, +1 for the grue

@jonasschnelli

View changes

Show outdated Hide outdated src/wallet/rpcdump.cpp
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Feb 19, 2016

Member

Nice and clean PR.
Concept ACK.

Member

jonasschnelli commented Feb 19, 2016

Nice and clean PR.
Concept ACK.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Feb 19, 2016

Member

And @paveljanik is right: importprunedfunds.py needs a -rwxr-xr-x file permissions mode.

Member

jonasschnelli commented Feb 19, 2016

And @paveljanik is right: importprunedfunds.py needs a -rwxr-xr-x file permissions mode.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Feb 19, 2016

Member

@sipa @gmaxwell

Seems the easiest way to do this is mark outputs as spent or not, and modify all functions which tally funds to account for this. For now the wallet simply checks if things are spent by looking through its wallet and looking for spends.

Member

instagibbs commented Feb 19, 2016

@sipa @gmaxwell

Seems the easiest way to do this is mark outputs as spent or not, and modify all functions which tally funds to account for this. For now the wallet simply checks if things are spent by looking through its wallet and looking for spends.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Feb 19, 2016

Member

Alternatively, when computing available funds we can check if each output is available directly in the utxo set, rather than (just?) looking at wallet transactions.

I don't know the internals well enough to know which is best, or if these are both terrible ideas. Especially in the presence of reorgs.

Member

instagibbs commented Feb 19, 2016

Alternatively, when computing available funds we can check if each output is available directly in the utxo set, rather than (just?) looking at wallet transactions.

I don't know the internals well enough to know which is best, or if these are both terrible ideas. Especially in the presence of reorgs.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Feb 19, 2016

Member
Member

sipa commented Feb 19, 2016

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Feb 19, 2016

Member

That was my feeling after digging around and thinking about it.
On Feb 19, 2016 12:33 PM, "Pieter Wuille" notifications@github.com wrote:

Marking outputs as spent is very complicated, as spendability depends on
whether other transactions exist that spend them, which themselves may be
in conflict with the blockchain.

We can check the UTXO set for spendability in theory, but that introduces
yet another dependency between the wallet and the node, and is something
that fundamentally requires a trusted full node.

IMHO, if you're manually importing transactions, you're bypassing the
entire sync mechanism, it's your responsibility to also import whatever
other transactions that may be relevant.


Reply to this email directly or view it on GitHub
#7558 (comment).

Member

instagibbs commented Feb 19, 2016

That was my feeling after digging around and thinking about it.
On Feb 19, 2016 12:33 PM, "Pieter Wuille" notifications@github.com wrote:

Marking outputs as spent is very complicated, as spendability depends on
whether other transactions exist that spend them, which themselves may be
in conflict with the blockchain.

We can check the UTXO set for spendability in theory, but that introduces
yet another dependency between the wallet and the node, and is something
that fundamentally requires a trusted full node.

IMHO, if you're manually importing transactions, you're bypassing the
entire sync mechanism, it's your responsibility to also import whatever
other transactions that may be relevant.


Reply to this email directly or view it on GitHub
#7558 (comment).

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Mar 7, 2016

Member

Added a companion RPC call to allow removal of imported transactions.

Member

instagibbs commented Mar 7, 2016

Added a companion RPC call to allow removal of imported transactions.

@instagibbs instagibbs changed the title from [RPC] Add importprunedfunds rpc call to [RPC] Add import/removeprunedfunds rpc call Mar 7, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 7, 2016

Member

Added a companion RPC call to allow removal of imported transactions.

Nice!

Member

laanwj commented Mar 7, 2016

Added a companion RPC call to allow removal of imported transactions.

Nice!

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Mar 10, 2016

Member

Fixed the undefined behavior that was causing the test to throw an error.

Member

instagibbs commented Mar 10, 2016

Fixed the undefined behavior that was causing the test to throw an error.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Mar 14, 2016

Member

Squashed.

Member

instagibbs commented Mar 14, 2016

Squashed.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Mar 23, 2016

Member

rebased

Member

instagibbs commented Mar 23, 2016

rebased

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 29, 2016

Member

utACK 7eb7029

Member

laanwj commented Mar 29, 2016

utACK 7eb7029

@laanwj laanwj merged commit f1bb13c into bitcoin:master Mar 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 29, 2016

Merge #7558: [RPC] Add import/removeprunedfunds rpc call
f1bb13c Added companion removeprunedfunds call. (instagibbs)
7eb7029 Add importprunedfunds rpc call (instagibbs)

@laanwj laanwj referenced this pull request Mar 29, 2016

Merged

Add importmulti RPC call #7551

@laanwj laanwj referenced this pull request Mar 30, 2016

Closed

TODO for release notes 0.13.0 #7678

14 of 16 tasks complete

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7558: [RPC] Add import/removeprunedfunds rpc call
f1bb13c Added companion removeprunedfunds call. (instagibbs)
7eb7029 Add importprunedfunds rpc call (instagibbs)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7558: [RPC] Add import/removeprunedfunds rpc call
f1bb13c Added companion removeprunedfunds call. (instagibbs)
7eb7029 Add importprunedfunds rpc call (instagibbs)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge #7558: [RPC] Add import/removeprunedfunds rpc call
f1bb13c Added companion removeprunedfunds call. (instagibbs)
7eb7029 Add importprunedfunds rpc call (instagibbs)

codablock added a commit to codablock/dash that referenced this pull request Dec 19, 2017

Merge #7558: [RPC] Add import/removeprunedfunds rpc call
f1bb13c Added companion removeprunedfunds call. (instagibbs)
7eb7029 Add importprunedfunds rpc call (instagibbs)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment