Skip to content
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

wallet: make it possible to disable transaction broadcast #5951

Merged
merged 2 commits into from Apr 8, 2015

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Mar 27, 2015

This is an advanced feature which will disable any kind of automatic transaction broadcasting in the wallet. It gives the user full control of how their transactions are sent.

They can broadcast a new transaction through some other mechanism, or send it to the recipient directly, after getting the transaction hex through gettransaction. This can be used to do #4564.

It is an important step toward addressing #3828.

This just adds the option -walletbroadcast=<0,1>. Right now with -walletbroadcast=0 new transactions will get the status

Status: conflicted, has not been successfully broadcast yet

...which is a tad strange. This is just a visual issue, though, and goes away when the transaction is received through the network.

Edit: discussing with @gmaxwell on IRC we thought of a way to avoid this without arcane changes to conflict handling: keep track of the seen-the-network status of transactions internally. If the transaction was not seen on the network, either through our own broadcasting or from a third-party, show it with a different status

@laanwj laanwj added the Wallet label Mar 27, 2015
@laanwj laanwj force-pushed the 2015_03_wallet_disable_tx_broadcast branch 4 times, most recently from 3e7c915 to cff1cf8 Compare March 27, 2015 16:04
@dgenr8
Copy link
Contributor

dgenr8 commented Mar 28, 2015

+1 to users not seeing the term "conflicted" when such is not the case. I don't know what else was considered but tracking a persistent seen-the-network status and showing "Not shared" or similar seems a good way to do this.

@gmaxwell
Copy link
Contributor

@dgenr8 I think it was never the plan to leave it that way forever... In any case, the more complete and correct way to handle that is to change the conflict logic from checking if the txn is in the mempool already to checking if the it would be accepted into the mempool if it were tried; but thats a fair bit of work for what is otherwise a small feature, so it's not great to have to do it first.

@laanwj
Copy link
Member Author

laanwj commented Mar 30, 2015

This functionality is already useful, temporarily showing the transaction as conflicted is arguably a small issue. Not saying it doesn't deserve to be fixed, but it could be done later.

@gmaxwell
Copy link
Contributor

@laanwj getrawtransaction needs to check the wallet. :) (otherwise there is no way to getrawtransaction on a transaction you just created via the wallet)

@jonasschnelli
Copy link
Contributor

utACK.
I think a rpc test who covers this feature would be nice. I'll write once during this or next week.

@gmaxwell
Copy link
Contributor

@jonasschnelli it doesn't actually work right now for the reason I described.

@laanwj
Copy link
Member Author

laanwj commented Apr 1, 2015

It does actually work right now. I've tested it.

As said in the opening post you can get the transaction hex using gettransaction. Using getrawtransaction won't work, as it doesn't (and should not!) query the wallet.

@gmaxwell
Copy link
Contributor

gmaxwell commented Apr 1, 2015

::facepalm:: I had completely forgotten about the existence of gettransaction and on account read that as getrawtransaction.

@gmaxwell
Copy link
Contributor

gmaxwell commented Apr 1, 2015

(tested) ACK.

@jonasschnelli
Copy link
Contributor

tested ACK.
rpc test: jonasschnelli@f63d3e9

laanwj and others added 2 commits April 1, 2015 13:03
This is an advanced feature which will disable any kind of automatic
transaction broadcasting in the wallet. This gives the user full control
of how the transaction is sent.

For example they can broadcast new transactions through some other
mechanism themselves, after getting the transaction hex through `gettransaction`.

This just adds the option `-walletbroadcast=<0,1>`. Right now these
transactions will get the status

    Status: conflicted, has not been successfully broadcast yet

They shouldn't be shown as conflicted at all (`walletconflicts` is empty). This status
will go away when the transaction is received through the network.
@laanwj laanwj force-pushed the 2015_03_wallet_disable_tx_broadcast branch from 41d08e0 to 77650cc Compare April 1, 2015 11:03
@laanwj
Copy link
Member Author

laanwj commented Apr 1, 2015

rebased and squashed to pull in @jonasschnelli 's RPC test, thanks!

@jgarzik
Copy link
Contributor

jgarzik commented Apr 1, 2015

ut ACK

@laanwj laanwj merged commit 77650cc into bitcoin:master Apr 8, 2015
laanwj added a commit that referenced this pull request Apr 8, 2015
77650cc add -walletbroadcast=0 rpc test (Jonas Schnelli)
6f25262 wallet: make it possible to disable transaction broadcast (Wladimir J. van der Laan)
@laanwj laanwj mentioned this pull request Apr 15, 2015
@laanwj laanwj added the Privacy label Aug 18, 2015
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 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.

None yet

5 participants