Concept ACK.

Concept ACK

Concept-ACK.

Should the sense of final and non-standard be inverted in the options? It would be surprising to pass a transaction to this RPC, have it pass.... then find that the network won't accept it. Practically speaking, except for special cases and developers standardness is pretty much as forceful as validity.

An obvious future extension wrt finality would be adding a field in the result that says when the transaction will become final.

@gmaxwell would it not be easier to make an optional parameter the estimated block height?
This would probably need to be specified for each transaction if we're passing a list (so you can do CSV chains etc).
Or would it be better to just give that feedback to the user as the "earliest possible block height" it could be included in? [per transaction]

edit: As suggested by @gmaxwell (I think), IMHO the best approach here is to give feedback to the user as to the earliest time they could be included.

it would be surprising to pass a transaction to this RPC, have it pass.... then find that the network won't accept it.

That's why those checks are enabled (true) by default. For specific cases they could be disabled. Only for the coinbase check I couldn't think of any reason why one'd want to disable it, so I left that out.

An obvious future extension wrt finality would be adding a field in the result that says when the transaction will become final.

Yes, would be nice - by using an object both for the options and the error, I made the API extensible enough to add such bells and whistles.
(either a "pass a simulated height", or "report when final" approach, or both, could fit in)

concept tested ACK, this is great for testing smart contract stuff with timelocks.

concept ack, but this is very incomplete, I suggest we keep it open without merging too fast.

We're arbitrarily calling IsStandardTx() but not AreInputsStandard(). Those are just two easily functions containing relay/ming policy, but there's much more related code spread out there.

Nit: Remove call to IsStandardTx() in verifytx, we can make another rpc call (or add an option in this one) called "verify tx according to consensus rules and also accept tx according to some confirgurable policy parameter". But please let's not add any policy code until we have a complete Consensus::VerifyTx().
Wait a minute, Consensus::VerifyTx() doesn't even exist on master yet, not in a complete form and neither in an incomplete form.

Nit: Let's please introduce Consensus::VerifyTx() before exposing anything else, otherwise you'll have to duplicate some code from AcceptToMemoryPool and/or ConnectBlock. Could you please consider rebasing this PR on top of #7565 ?

IMHO agreed this would be best implemented by Consensus::VerifyTx or the like.

I also like the idea of a parameter for rejection based purely on consensus, and irrespective of local node policy.

@jtimon I don't feel like doing all kinds of refactors here (too much risk to break other things). Please just review this for correctness.

I also like the idea of a parameter for rejection based purely on consensus, and irrespective of local node policy.

Right - that's possible already. Disable check_standard, and possible check_final, or am I missing something?

Fair enough, concept ack.
I would still prefer a more generic and extensible option than bool check_standard (string policy="default"/"none" ?).
I still don't know why isstandard is called but areinputsstandard isn't. There's more to-be-consensus code out of here (that's what I mean by this is incomplete ).

I still don't know why isstandard is called but areinputsstandard isn't.

Yes, that's a legit bug (I added it to TODOs in the opening post)

I would still prefer a more generic and extensible option than bool check_standard (string policy="default"/"none" ?).

What other options would you imagine there? Adding more options to the options structure is free, so there could always be a "standard_policy": "string" added later.

I've factored out the transaction verification to a (local) VerifyTransaction function.

Too much disagreement on how this should be implemented, not going to bother anymore.

I don’t understand why this is closed. I asked to wait for some refactors first. You said no, fair enough, they can be done later. Then you create the verifyTx function, but not in the consensus package or dir, and including policy code. I can't say I like that (I'm not going to link to the VerifyTx I like anymore because as said it requieres refactors and I was waiting for 0.12.1 first. I guess I wouldn't mind if the function was called VerifyAndAcceptTx or something like that.

But why not continue with your original version? Wasn't I the only person complaining and didn't I said fair enough and concept ACK?
I really can't see "too much disagreement here, unless you're talking about some conversation on IRC or somewhere else. If that's the case, a link to the log of the conversation would be welcomed here for traceability.

But why not continue with your original version? Wasn't I the only person complaining and didn't I said fair enough and concept ACK?
I really can't see "too much disagreement here, unless you're talking about some conversation on IRC

Well someone else on IRC was worried that the output of this doesn't exactly match the output of sendrawtransaction.

<runeks> wumpus: Awesome! Looks like just what I need.
<runeks> But, as others in the pull request have mentioned, we better be damn sure that its return result is *always* the same as that of sendrawtransaction.
<runeks> Looking at the code, to me it doesn't seem trivial achieve this.
<runeks> Implementation-wise, it would seem a lot more robust to use the same code path as sendrawtransaction, but abort before the transaction is committed, and just return "valid".
<wumpus> that would be a non-trivial impact to the current transaction handling code
<wumpus> not a good idea with all the softforks etc going on
<wumpus> anyhow, closing it then, feel free to implement it in a way you think is better
<wumpus> I thought this was a good way to get the API merged, it could always be improved later
<wumpus> also the 'verify multiple transactions' part would be more difficult to implement if you want to bolt this onto main.cpp

I don't however want to just bolt this on to main.cpp in the current state. Without refactors that'd make the code even harder to understand, and makes it virtually impossible to handle multiple dependent transactions without actually writing to the mempool.
(and the mempool logic in AcceptToMempool really gets in the way, e.g. the replacment logic)

So if this solution is not acceptable for the mean time, I suppose we should wait for those refactors to materialize first.

Thank you.

Why the additional requirement of this having to return the same as sendrawtx?

Wouldn't everything be much easier if VerifyTx ONLY did consensus checks? As said we can have a VerifyAndAcceptTx later thatalso does relay/mempool policy stuff as well (but, yeah, I agree the policy code needs many refactors if you want to use any of the fee-related code in the CtxMempool and AcceptToMemoryPool from here).

Why the additional requirement of this having to return the same as sendrawtx?

That is the default assumption. I suppose if we can document the differences (e.g. no mempool replacement, no fee checks) that'd help.

If you want to do only consensus checks you can already do that with this code, just disable the standardness check (and possibly the use mempool flag). I do think checking standardness etc is useful in some cases though.

Well, yeah, documenting the differences seems much simpler to make the relay/mempool policy checks complete. So called "standardness" is encapsulated in policy/policy.o and its dependencies, but completing the rest of the policy checks won't be so simple I think.
I would rather complete consensus or to-be-consensus checks first (ie bips 68, 112 and 113).
With or without "standardness", I may backport this code to https://github.com/jtimon/bitcoin/tree/jt (after I rebase it on top of bip9, etc) to see if it can help me make some point about encapsulation or initiate some API discussion at the rpc level (something that I've failed to achieve at the C API level, with, say, jtimon@11fa11a ).

If anyone else wants to continue this for master directly, I'm happy to re-review, though.

Wouldn't the easiest way to make this feature is to create a dummy block, with a dummy coinbase, and add the transaction inside it and call ConnectBlock with fJustCheck to true ?

I don't think the goal of this PR was offering the functionality the "easiest way", but since it was checking policy things as well, I'm not sure I ever understood the PR.
In any case, what you propose (which doesn't sound very elegant) would also not be complete: currently you would need to also call ContextualCheckBlock() with that dummy block for the tx to be fully verified.

At the API level, I think users care only about one parameter: Whether we check against Node Policy or Consensus Policy. I think the current PR was attempting too many options.

I'll probably try to make it later since this PR already did most of the heavy work.

Wouldn't the easiest way to make this feature is to create a dummy block, with a dummy coinbase, and add the transaction inside it and call ConnectBlock with fJustCheck to true ?

Originally I meant this as a pre-check whether sendrawtransaction will accept a chain of transactions.
Creating a dummy block would work to check consensus validity only.

I think the current PR was attempting too many options.

Only three options! And I think all of them make sense in some context.