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

RPC: minetxlocally #21372

Closed
wants to merge 2 commits into from
Closed

RPC: minetxlocally #21372

wants to merge 2 commits into from

Conversation

DesWurstes
Copy link
Contributor

@DesWurstes DesWurstes commented Mar 5, 2021

Function: sendrawtransaction + prioritiserawtransaction - relay=true - policy

Rationale: I guess that many pool operators have implemented this in their forks already. Adding it here makes it more convenient for them.

This allows them to mine their high-fee/nonstandard transactions for free.

For example, if a miner needs to consolidate lots of outputs, this allows them to finish this in one large tx and for free.

I might need to rebase it after #21061

Would you Concept ACK it?

I need to add tests so that even https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_segwit.py#L1466-L1468 passes with this feature enabled.

@gmaxwell
Copy link
Contributor

gmaxwell commented Mar 5, 2021

Doing this is pretty bad for block propagation performance-- probably adding over 100ms to the global propagation time because it guarantees a round trip at every hop. ... especially now with fibre mostly gone.

Last I looked the only pool mining there own non-standard transactions was f2pool (>100kb in their case).

This facility would also make it extremely easy to unintentionally fork the network by mining transactions which use functionality which is reserved for future consensus rules which may already be enforced by part of the network. Particularly because it's not realistic for any external software to enforce softfork safeness, since you need a whole script interpreter to do so -- an obvious thing someone might try to do with this interface is make a "transaction accelerator" but in doing so they'd accidentally introduce a public facing network split generator.

@DesWurstes
Copy link
Contributor Author

DesWurstes commented Mar 5, 2021

Doing this is pretty bad for block propagation performance-- probably adding over 100ms to the global propagation time because it guarantees a round trip at every hop. ... especially now with fibre mostly gone.

I don't get this part, are you referring to Compact Blocks? Mining pool nodes receive full blocks from each other with transactions, adding an unbroadcast transaction wouldn't break anything.

I aimed to give the miner more control by this change (not every miner has a fork of Core) and now it seems that the best is to add a warning to the RPC description since the last thing a miner wants is a network fork.

I don't think this feature can be abused, it's only for convenience. For an evil miner, this doesn't make anything easier. They can remove IsStandard even now.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 5, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@sipa
Copy link
Member

sipa commented Mar 5, 2021

I don't get this part, are you referring to Compact Blocks?

Compact blocks are able to relay blocks without any roundtrips if all the blocks' transactions (excluding the coinbase) are known to the recipients already. For sufficiently long-running nodes, in practice this is very often the case. As soon as there is at least one transaction missing, a roundtrip is needed which effectively triples the block relay time on each hop (CMPCTBLOCK-GETBLOCKTXN-BLOCKTXN instead of just CMPCTBLOCK).

So generally, adding transactions to your own blocks without even (trying to) relay them to the network is a bad idea for the network, and for the miner personally, as they have an incentive to get their blocks propagated as swiftly as possible to other miners.

@DesWurstes
Copy link
Contributor Author

DesWurstes commented Mar 6, 2021

                "\nInclude a raw transaction in the next block without relaying it.\n"
                "WARNING: Mining a transaction that is not relayed will slow down the propoagation of the block\n"
                "and increase the probability of being orphaned! Prefer sendrawtransaction + prioritisetransaction\n"
                "whenever possible.",

I think this still contributes to the functionality.

@gmaxwell
Copy link
Contributor

gmaxwell commented Mar 6, 2021

I don't think this feature can be abused, it's only for convenience.

It's extremely dangerous because the consensus rules are subtle and no reasonable user expects an exposed option that may cause their blocks to be rejected as invalid.

Nothing in my comment said anything about malice by miners, in fact I specifically said "unintentionally". Most causes of non-standardness are intentional reservations for future consensus rules, if miners mine them it makes it much harder to safely update the consensus rules and risks causing their blocks to get orphaned.

There are policy elements that can be safely bypassed, e.g. min-fee or dust limit-- so someone intentionally bypassing for those reasons would inadvertently bypass other policy checks and potentially fork the consensus and get themselves orphaned. And again, you cannot validate these rules for yourself externally without essentially a copy of the script interpreter.

The fact that someone could edit the software and footgun themselves is not an argument for exposing functionality that has a bad, harmful, and dangerous practice built in. It's an argument against it-- if anything-- because if there is a legitimate reason then there is still a way.

@DesWurstes
Copy link
Contributor Author

convinced👍

@DesWurstes DesWurstes closed this Mar 6, 2021
@maflcko
Copy link
Member

maflcko commented Mar 7, 2021

(slightly related to #20753 )

@luke-jr
Copy link
Member

luke-jr commented Mar 9, 2021

As soon as there is at least one transaction missing, a roundtrip is needed which effectively triples the block relay time on each hop

It doesn't need to. Maybe we should add logic to include any policy-rejected transactions in the initial compact block?

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
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

6 participants