Network ID logic should not be applied to pseudo-transactions

[dangell7]

This PR fixes the issue with enabling new amendments on a network with an ID > 1024.

See an example here: #4736

• Context on pseudo-transactions

[intelliot]

Just curious - is there a reason this solution is better than adding the NetworkID field to pseudo-transactions?

[mvadari]

Just curious - is there a reason this solution is better than adding the NetworkID field to pseudo-transactions?

I don't fully understand how pseudo-transactions work, but is there a chance that a signed pseudo-transaction could be submitted on another network to cause unwanted changes?

[ximinez]

Pseudo-transactions don't get signed, nor are they submitted. I think it's possible to request one, but I'm not 100% sure of that. Point is, that's not a risk.

That said, adding the NetworkID to pseudo-transactions should be an equally viable solution. Perhaps better, because someone seeing one of these pseudo-txs would know exactly where it came from.

P.S. Either way, unit tests should be added to ensure that pseudo-transactions are generated and processed correctly with a network ID > 1024.

[RichardAH]

That said, adding the NetworkID to pseudo-transactions should be an equally viable solution. Perhaps better, because someone seeing one of these pseudo-txs would know exactly where it came from.

P.S. Either way, unit tests should be added to ensure that pseudo-transactions are generated and processed correctly with a network ID > 1024.

It's not obvious to me why pseudos would ever need a network ID? If you want to differentiate networks just add NetworkID to the fees (or another singleton) object?

[intelliot]

It would never be needed. When looking some pseudo-transaction JSON, you would be able to see which network it came from, however.

[gregtatcam]

It's better not to have the empty block and check if (!isPseudoTx(ctx.tx))

[gregtatcam]

I'm not having much luck with testing this fix. I run a local network of 5 nodes. I added this to config:

 [network_id]
 1026
 [amendment_majority_time]
 16 minutes

I ran this to every node:

         {
             "method": "feature",
             "params": [
               {
                 "feature": "AMM",
                 "vetoed": false
               }
             ]
           }

This is the response:

  {'count': 1,
    'enabled': False,
    'name': 'AMM',
    'supported': True,
    'threshold': 3,
    'validations': 5,
    'vetoed': False}

[intelliot]

@gregtatcam did you wait for at least 2 * amendment_majority_time? I think you may also need at least 2 flag ledgers (1 for "got majority" and 1 for "enabled") which usually takes at least 30 minutes. (To be safe, maybe check after 1 hour.)

[gregtatcam]

I'll re-test with the suggested wait time to double check.

[gregtatcam]

I waited 45 minutes - the same result.

[intelliot]

Notes

• Concern: consistency, and having all transactions follow the same rules
• May be preferable to avoid making exceptions which may be forgotten in the future
• Should try to minimize exceptions that apply only to pseudo-transactions

[intelliot]

Just to close the loop on my earlier question: I'm personally fine with this solution, given these are pseudo-transactions, which are already different from normal transactions anyway. It makes sense to review (and potentially merge) this change as-is.

[seelabs]

What should we do if a pseudo transaction has a network id? Right now it's ignored, which is probably fine. But some other options would be:

1. If present, it needs to follow the same rules as other transactions (i.e. match the configured network id), fail if the network is less than 1024.
2. Fail if present in any form.
3. Assert if present in any form, but allow to continue processing in release mode.

I'd probably prefer (3). If someone does add a network id to a pseudo transaction, and they did it intentionally, the assert will let them know this code needs to change. But your call if you want to add it or not. I'm fine either way.

As a side note, I also prefer not to have the empty if block, but that's just a preference. I'm also fine if you want to keep it as-is.

[ximinez]

I'd vote for option 1. It has the advantage of being simple to add that check. Simply change the check to

if (isPseudoTx(ctx.tx) && !ctx.tx.isFieldPresent(sfNetworkID))

[dangell7]

I like option 1 because its simple.

[seelabs]

@dangell7 I don't think that patch is going to work. If the network ID is required, but isn't present the new patch lets the transaction succeed.

[seelabs]

I think @ximinez is right. The block should look something like:

    if (!isPseudoTx(ctx.tx) || ctx.tx.isFieldPresent(sfNetworkID))
    {
        uint32_t nodeNID = ctx.app.config().NETWORK_ID;
        std::optional<uint32_t> txNID = ctx.tx[~sfNetworkID];

        if (nodeNID <= 1024)
        {
            // legacy networks have ids less than 1024, these networks cannot
            // specify NetworkID in txn
            if (txNID)
                return telNETWORK_ID_MAKES_TX_NON_CANONICAL;
        }
        else
        {
            // new networks both require the field to be present and require it
            // to match
            if (!txNID)
                return telREQUIRES_NETWORK_ID;

            if (*txNID != nodeNID)
                return telWRONG_NETWORK;
        }
    }

[dangell7]

fixed in d8b2ba4

[intelliot]

notes:

• could not verify in testing. an issue with environment? Michael may be trying to test. @manojsdoshi will connect with @gregtatcam to see why prior test attempts did not work.
• once merged, Manoj and Ram will test with a real test network.
• need to include this for sidechains.

[seelabs]

👍

[gregtatcam]

👍

[intelliot]

note: needs to be brought up-to-date with develop

[sgramkumar]

Verified on networks with network_id 12 & 1212. Amendment is enabled in both cases.