Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Sep 26, 2015

This fixes #6725 by limiting the scope of REJECT_HIGHFEE back to raw transactions (as intended by it's original author).

@laanwj
Copy link
Member

laanwj commented Sep 26, 2015

Concept-level:

a) Should we be disabling sanity checks for our own transactions? It provides a level of fallback safety. HIGHFEE check exists for a good reason

b) We shouldn't be generating transactions that our own mempool doesn's accept: good chance that other nodes won't accept them either

@laanwj laanwj added the Wallet label Sep 26, 2015
@maflcko
Copy link
Member Author

maflcko commented Sep 26, 2015

@laanwj "sanity checks for our own [wallet created] transactions ..."

... should be executed by the wallet code. (Which it already does)

@laanwj "good chance that other nodes won't accept them either".

This PR only fixes the fee-sanity-check. And other nodes accepting txs over the network don't care about (high) fees when adding them to their own mempool. (I can add test cases to this PR if you want)

@sipa
Copy link
Member

sipa commented Sep 26, 2015 via email

@jonasschnelli
Copy link
Contributor

I understand the motivation of this PR.
Does it make sense to have this absurde-fee-checks in the mempool logic? I think the mempool should not enforce a such policy.

Conceptual i think having a function that could check if a transaction would be accepted to the mempool (or AcceptToMemoryPool() with a dontAdd boolean).

IMO the wallets send functions as well as the sendrawtransaction RPC call should do the necessary checks.

@maflcko
Copy link
Member Author

maflcko commented Sep 28, 2015

@jonasschnelli Does it make sense to have this absurde-fee-checks in the mempool logic?

No, see OP of #2949: The only reason to do this is "because the inputs may not be known".

Though, I'd rather not bloat this PR and leave the dry-run logic (or whatever is better) to a follow up PR.

@morcos
Copy link
Contributor

morcos commented Oct 19, 2015

@MarcoFalke I don't understand this:

@laanwj "sanity checks for our own [wallet created] transactions ..."

... should be executed by the wallet code. (Which it already does)

It looks to me like you're removing any sanity checking at all from the wallet code. Although there is still some from the GUI. see #5202.

As the author of the fee estimation code, I can tell you I would not feel comfortable if there was not a hard coded (or better yet configurable) high fee check.

@laanwj
Copy link
Member

laanwj commented Nov 5, 2015

Nack - see discussion in #6887, I don't like removing highfee checks

@laanwj laanwj closed this Nov 5, 2015
@maflcko maflcko deleted the MarcoFalke-2015-rejectWtxFix branch November 5, 2015 17:48
@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mempool should not reject wallet created transactions with sufficient fee
5 participants