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

Fix resendwallettransactions assert failure if -walletbroadcast=0 #10995

Merged
merged 1 commit into from Aug 7, 2017

Conversation

Projects
None yet
6 participants
@TheBlueMatt
Contributor

TheBlueMatt commented Aug 5, 2017

This fixes #10981 in my preferred way.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 5, 2017

Member

utACK

Member

laanwj commented Aug 5, 2017

utACK

@laanwj laanwj added this to the 0.15.0 milestone Aug 5, 2017

@laanwj laanwj added the Wallet label Aug 5, 2017

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 5, 2017

Member

utACK 48ff182

Member

sipa commented Aug 5, 2017

utACK 48ff182

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Aug 6, 2017

Member

Even though resendwallettransactions is for testing purpose, having an empty response indicates that there are no unconfirmed transactions, which can be incorrect. IMO it should return an error like, for instance, RPC_INVALID_REQUEST.

Member

promag commented Aug 6, 2017

Even though resendwallettransactions is for testing purpose, having an empty response indicates that there are no unconfirmed transactions, which can be incorrect. IMO it should return an error like, for instance, RPC_INVALID_REQUEST.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Aug 7, 2017

Contributor

@promag seems reasonable, I added a commit to do so.

Contributor

TheBlueMatt commented Aug 7, 2017

@promag seems reasonable, I added a commit to do so.

@promag

Concept ACK modulus comments.

Show outdated Hide outdated src/wallet/rpcwallet.cpp Outdated
Show outdated Hide outdated src/wallet/wallet.cpp Outdated
@@ -2589,6 +2589,7 @@ UniValue resendwallettransactions(const JSONRPCRequest& request)
"Immediately re-broadcast unconfirmed wallet transactions to all peers.\n"
"Intended only for testing; the wallet code periodically re-broadcasts\n"
"automatically.\n"
"Returns an RPC error if -walletbroadcast is set to false.\n"

This comment has been minimized.

@promag

promag Aug 7, 2017

Member

In the help you reference the app arg, but not in the error message. Make them consistent?

@promag

promag Aug 7, 2017

Member

In the help you reference the app arg, but not in the error message. Make them consistent?

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Aug 7, 2017

Contributor

Fixed issues and squashed.

Contributor

TheBlueMatt commented Aug 7, 2017

Fixed issues and squashed.

@laanwj laanwj merged commit 01699fb into bitcoin:master Aug 7, 2017

1 check passed

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

laanwj added a commit that referenced this pull request Aug 7, 2017

Merge #10995: Fix resendwallettransactions assert failure if -walletb…
…roadcast=0

01699fb Fix resendwallettransactions assert failure if -walletbroadcast=0 (Matt Corallo)

Pull request description:

  This fixes #10981 in my preferred way.

Tree-SHA512: 2e43d3ac78d13c5d59db23a82c76c722cc3344767a8237617080e489296d27a98bb1b3bd469b2c9b289b57a9da3709c90448d7a23bcc2e1dfb791c4fd16be015
@dougstrong77

This comment has been minimized.

Show comment
Hide comment
@dougstrong77

dougstrong77 Aug 7, 2017

Plz send me some where to learn this im confused and screwing up everything

dougstrong77 commented Aug 7, 2017

Plz send me some where to learn this im confused and screwing up everything

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Aug 7, 2017

Member

Post-merge comment: application-level errors should not use RPC_INVALID_REQUEST. Fixed in #11002.

Member

jnewbery commented Aug 7, 2017

Post-merge comment: application-level errors should not use RPC_INVALID_REQUEST. Fixed in #11002.

laanwj added a commit that referenced this pull request Aug 8, 2017

Merge #11002: [wallet] return correct error code from resendwallettra…
…nsaction

055d95f [wallet] return correct error code from resendwallettransaction (John Newbery)

Pull request description:

  New code in #10995 uses `RPC_INVALID_REQUEST`. According to the comment in rpc/protocol.h:
  ```
  // RPC_INVALID_REQUEST is internally mapped to HTTP_BAD_REQUEST (400).
  // It should not be used for application-layer errors.
  ```
  Change the returned error code to `RPC_WALLET_ERROR`

  #11000 will need to be updated to test for the correct error code.

Tree-SHA512: 0201b3a2091adf17ad301825da5bd29f0ea7e284b5394cbef80483fc293a558acc849f74a0780bb8501acab324fc722e41ae049cffec7afb76884e26df4b809e

MarcoFalke added a commit that referenced this pull request Aug 12, 2017

Merge #11000: test: Add resendwallettransactions functional tests
bdf607e test: Add resendwallettransactions functional tests (João Barbosa)

Pull request description:

  Adds functional tests to cover the behaviour introduced in #10995.

Tree-SHA512: 3be337cbe51edab2cc0eb9ecfa68d00cd3c3a00aef0672cd841706802726c9cd4138626a8f209c3d9fbd207ce332daa062f270e4c94c1c2398bfc475e36423f6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment