I suggest "mempooltxinputlimit"; vin is internal jargon.

Also, it's the number of transparent inputs.

Done

Also change this if vin is dropped from the option name.

Done

I believe the default should be 40, but let's discuss that on #2343.

Default is now 0 which means no limit (code has changed in forced push)

Have adopted language.

Done

Non-blocking: s/which/that/

Done

s/cannot not/cannot/

Done

Why are all the error messages in the test related to 'bad-txns-version-too-low' ? I only see it fired here:

bool CheckTransactionWithoutProofVerification(const CTransaction& tx, CValidationState &state
)
{
    // Basic checks that don't depend on any context

    // Check transaction version
    if (tx.nVersion < MIN_TX_VERSION) {
        return state.DoS(100, error("CheckTransaction(): version too low"),
                         REJECT_INVALID, "bad-txns-version-too-low");
    }

Correct. I intentionally set tx.nVersion = 0 to trigger this error, as it's the first check that occurs after the -mempooltxinputlimit check, and it means that I didn't have to mock out a heap of global state. Think of that error as "the transaction passes the tx input limit check". The actual check itself doesn't return a reason, which is why I check that the reject reason is not bad-txns-version-too-low; we could possibly make things more explicit by checking instead that it is empty.

Could you copy the above explanation into the test? I take it we don't want to return state.DoS which updates global CValidationState state in init.cpp, rather than what we do right now which is simply return false (like some of the other code paths in AcceptToMemoryPool).

At this point in the function, this is the list of all available inputs, not selected inputs; z_sendmany therefore fails incorrectly. This check needs to be shifted to just below where t_inputs_ is re-used for storing the selected inputs. We also need to re-write this code (likely as part of #1360) to prevent this kind of bug.

Fixed

Wait, input limiting occurs before the tx version check? How can we parse the transaction if it has an unknown version? The spec explicitly says you're not supposed to try to parse fields other than nVersion if the version is unrecognised. (This could lead to misreporting a transaction as being invalid for having too many inputs, when in fact that field at that position has changed its meaning entirely and the transaction should instead have been rejected for having an unknown version. Not critical, but it could lead to confusing log messages.)

I did wonder this myself. However:

• The version check is solely that tx.nVersion >= MIN_TX_VERSION, where MIN_TX_VERSION = 1, so any positive integer is already accepted. This is in fact one way in which transaction replay prevention has been proposed upstream: make the version negative.
• Transaction parsing already happens before AcceptToMemoryPool is called, so the spec should still be satisfied:

bool static ProcessMessage(...)
{
    ...
    else if (strCommand == "tx")
    {
        vector<uint256> vWorkQueue;
        vector<uint256> vEraseQueue;
        CTransaction tx;
        vRecv >> tx;

        CInv inv(MSG_TX, tx.GetHash());
        pfrom->AddInventoryKnown(inv);

        LOCK(cs_main);

        bool fMissingInputs = false;
        CValidationState state;

        pfrom->setAskFor.erase(inv.hash);
        mapAlreadyAskedFor.erase(inv);

        if (!AlreadyHave(inv) && AcceptToMemoryPool(mempool, state, tx, true, &fMissingInputs))
        {
            ...
        }
        ...
    }
    ...
}

bool ProcessMessages(...)
{
    ...
        bool fRet = false;
        try
        {
            fRet = ProcessMessage(pfrom, strCommand, vRecv, msg.nTime);
            boost::this_thread::interruption_point();
        }
        catch (const std::ios_base::failure& e)
        {
            ...
        }
    ...
}

A bystander note here, feel free to ignore, but could this reasoning be added as a comment to the code? I think it would greatly help others too and later when one is perhaps considering refactoring the code.

Thanks for the reminder @veikkoeeva - I've updated the first post in the conversation above, so when this is merged, it will also be in the commit message for the merge. I think anybody in the future who wants to refactor the code around mempooltxinputlimit will have the information on hand to do so.

I don't see this in the first post.

@daira You don't see the text beginning "Implement short-term... " ? Also the PR number should be part of the zkbot commit message, so a developer can find their way to this conversation.

The first call was changed from sendtoaddress to sendfrom; this one should be too.

ACK; the first variant was left over from an earlier version where node 1 already had a balance, so I was trying to get everything into one t-addr, but couldn't make multiple TXOs in the same z_sendmany.