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: sendrawtransaction: Allow the user to ignore/override specific rejections #7533

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@luke-jr
Member

luke-jr commented Feb 14, 2016

Replace boolean allowhighfees with an Array of rejections to ignore (in a backward compatible manner)

This is useful for node operators who wish to manually accept transactions that don't meet their typical policies, yet don't necessarily want to override all the policies.

It's a bit ugly internally - suggestions on improving that are welcome.

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 14, 2016

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 14, 2016

AcceptToMemoryPool: Refactor overrideable rejections (previously fOve…
…rrideMempoolLimit and fRejectAbsurdFee) with flexible std::set of rejections to ignore

Github-Pull: #7533
Rebased-From: 9f03c06

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 14, 2016

RPC: sendrawtransaction: Replace boolean allowhighfees with an Array …
…of rejections to ignore (in a backward compatible manner)

Github-Pull: #7533
Rebased-From: 1cc1ce4

@laanwj laanwj added the RPC/REST/ZMQ label Feb 15, 2016

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 25, 2016

rpc: Input-from-stdin mode for bitcoin-cli
Implements #7442 by adding an option `-stdin` which reads
additional arguments from stdin, one per line.

For example

```bash
echo -e "mysecretcode\n120" | src/bitcoin-cli -stdin walletpassphrase
echo -e "walletpassphrase\nmysecretcode\n120" | src/bitcoin-cli -stdin
```

Github-Pull: #7533
Rebased-From: 92bcca3
@jameshilliard

This comment has been minimized.

Show comment
Hide comment
@jameshilliard

jameshilliard Mar 9, 2016

Contributor

concept ACK

Contributor

jameshilliard commented Mar 9, 2016

concept ACK

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Mar 9, 2016

Member

@luke-jr why not turn the second argument an JSON object to be more scalable. For instance, I was planning to add option unlockUnspents.

Member

promag commented Mar 9, 2016

@luke-jr why not turn the second argument an JSON object to be more scalable. For instance, I was planning to add option unlockUnspents.

Show outdated Hide outdated src/main.h

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 28, 2016

@jameshilliard

This comment has been minimized.

Show comment
Hide comment
@jameshilliard

jameshilliard Jul 5, 2016

Contributor

Is this something that we might be able to get in 0.13? Without something like this the alternative for pool operators is usually to patch out the problematic is-standard check blocking the send which is usually not desired.

Contributor

jameshilliard commented Jul 5, 2016

Is this something that we might be able to get in 0.13? Without something like this the alternative for pool operators is usually to patch out the problematic is-standard check blocking the send which is usually not desired.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 11, 2016

Member

Sorry, no, this missed the feature freeze for 0.13 by a long haul.

Member

laanwj commented Jul 11, 2016

Sorry, no, this missed the feature freeze for 0.13 by a long haul.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 22, 2016

Member

Needs rebase on top of master instead of 0.13

Member

MarcoFalke commented Oct 22, 2016

Needs rebase on top of master instead of 0.13

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Nov 12, 2016

Member

@jameshilliard Knots has had this for a while. Miners should probably be using it anyway.

@MarcoFalke Rebased.

Member

luke-jr commented Nov 12, 2016

@jameshilliard Knots has had this for a while. Miners should probably be using it anyway.

@MarcoFalke Rebased.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Dec 10, 2016

Contributor

Concept ACK
Needs rebase.

Contributor

paveljanik commented Dec 10, 2016

Concept ACK
Needs rebase.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Dec 26, 2016

Member

Rebased.

Could be combined with #9422 to restore policy-bypassing transactions after a restart, but I consider that beyond the scope of this PR, and something to address after both get merged.

Member

luke-jr commented Dec 26, 2016

Rebased.

Could be combined with #9422 to restore policy-bypassing transactions after a restart, but I consider that beyond the scope of this PR, and something to address after both get merged.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 9, 2017

Member

I'm not convinced about the need to ignore based on the exact reason (as that is likely something that's hard to maintain, as reasons change over time). How about just a boolean to bypass standardness/fee/mempool policy rules (but keep consensus and script execution flags for upgradability)?

Member

sipa commented Apr 9, 2017

I'm not convinced about the need to ignore based on the exact reason (as that is likely something that's hard to maintain, as reasons change over time). How about just a boolean to bypass standardness/fee/mempool policy rules (but keep consensus and script execution flags for upgradability)?

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 11, 2017

Contributor

Agree with @sipa. This is not going to be maintainable for API clients. Are you planning on rebasing this luke?

Contributor

TheBlueMatt commented Jul 11, 2017

Agree with @sipa. This is not going to be maintainable for API clients. Are you planning on rebasing this luke?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Aug 14, 2017

Member

Typically people only want to bypass a specific policy, and not others. For example, a miner might want to bypass the fee checks or bypass the ancestor limit on replacements, but not other policies.

Will have a rebase done soon.

Member

luke-jr commented Aug 14, 2017

Typically people only want to bypass a specific policy, and not others. For example, a miner might want to bypass the fee checks or bypass the ancestor limit on replacements, but not other policies.

Will have a rebase done soon.

@jameshilliard

This comment has been minimized.

Show comment
Hide comment
@jameshilliard

jameshilliard Aug 14, 2017

Contributor

I don't think it's all that important to have the ability to have granular overrides, if that's important to some miners it can be implemented externally, most of the time a miner will just want to force add the transaction and will have already checked that it violates policy rules(often by looking at the failure reason when they try and send it normally).

Contributor

jameshilliard commented Aug 14, 2017

I don't think it's all that important to have the ability to have granular overrides, if that's important to some miners it can be implemented externally, most of the time a miner will just want to force add the transaction and will have already checked that it violates policy rules(often by looking at the failure reason when they try and send it normally).

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Aug 16, 2017

Member

@jameshilliard It can't be added externally... Looking at the first failure reason won't tell you if it violates other policies as well.

Anyhow, rebase is ready for review, and refactored somewhat so hopefully it's also easier to review.

Member

luke-jr commented Aug 16, 2017

@jameshilliard It can't be added externally... Looking at the first failure reason won't tell you if it violates other policies as well.

Anyhow, rebase is ready for review, and refactored somewhat so hopefully it's also easier to review.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Sep 21, 2017

Member

I think the ability to override specific rejection reasons is overkill, and risks creating an interface that is unmaintainable.

A boolean to say "ignore all policy, accept if consensus-valid" would be fine, though.

Member

sipa commented Sep 21, 2017

I think the ability to override specific rejection reasons is overkill, and risks creating an interface that is unmaintainable.

A boolean to say "ignore all policy, accept if consensus-valid" would be fine, though.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 21, 2017

Member

Agree with @sipa, making this too granular makes this unmaintainable as rejection reasons might come and go, or implemented differently, as policy changes. After all, policy is not standardized.
(on the other hand ,the interface would be expected to change based on policy changes...)

Member

laanwj commented Sep 21, 2017

Agree with @sipa, making this too granular makes this unmaintainable as rejection reasons might come and go, or implemented differently, as policy changes. After all, policy is not standardized.
(on the other hand ,the interface would be expected to change based on policy changes...)

@DuncanBetts

This comment has been minimized.

Show comment
Hide comment
@DuncanBetts

DuncanBetts Jan 8, 2018

I've just been mulling over this. How about giving node operators a hook, and allowing them to implement this sort of functionality themselves if they need it. I note that no node operators aside luke have requested this functionality here. I don't know how practical my suggestion is.

DuncanBetts commented Jan 8, 2018

I've just been mulling over this. How about giving node operators a hook, and allowing them to implement this sort of functionality themselves if they need it. I note that no node operators aside luke have requested this functionality here. I don't know how practical my suggestion is.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 25, 2018

this seems kool lets merge it

ghost commented Jan 25, 2018

this seems kool lets merge it

@Sjors

This comment has been minimized.

Show comment
Hide comment
@Sjors

Sjors Mar 8, 2018

Member

Needs love (as in, needs rebase and addressing concerns). A simpler alternative is probably the best way to move this forward. Some examples of use cases would be nice too; "node operators who wish to manually accept transactions that don't meet their typical policies" is a bit vague.

Member

Sjors commented Mar 8, 2018

Needs love (as in, needs rebase and addressing concerns). A simpler alternative is probably the best way to move this forward. Some examples of use cases would be nice too; "node operators who wish to manually accept transactions that don't meet their typical policies" is a bit vague.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 31, 2018

Member

Closing and adding "up for grabs", this is the oldest PR and has been inactive for quite long, too

Member

laanwj commented Aug 31, 2018

Closing and adding "up for grabs", this is the oldest PR and has been inactive for quite long, too

@laanwj laanwj closed this Aug 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment