PPC (peercoin) segwit transactions are rejected

[SirSevenG]

[placeholder for the issue, will update when get more data from synced PPC deamon @smk762]
Describe the bug
Trying to send from PPC-segwit with mm2 results in transaction being rejected.

ref: https://github.com/KomodoPlatform/WebDEX/pull/793#issuecomment-1402310411
Other attempt:

rpc:152] rpc:211] dispatcher_legacy:141] lp_coins:3045] utxo_common:2293] rpc_clients:2185] JsonRpcError { client_info: "coin: PPC-segwit", request: JsonRpcRequest { jsonrpc: "2.0", id: "53", method: "blockchain.transaction.broadcast", params: [String("01000000000101458184848727618027be38febdf1203ebd57c44913a352ebb6106d1ae4a1c6b90000000000ffffffff0210270000000000001976a9141bdefe84a64de8f0929689dd38dc9b4e865bd2bd88acb6c21100000000001600141bdefe84a64de8f0929689dd38dc9b4e865bd2bd0247304402205a9a2cd79484077c0a85f2cba2a68cc6af01b98731133cabcc81827e2294557b022040eb5b92f885aa49a8a833b6e1006bc19b2689dbed68484797eb534709252bab0121036b88f059d24b2641c0c324e1e5f513b3623294e98e5b17db20c49df1d07d1aeb6f04d163")] }, error: Response(wss://electrum.peercoinexplorer.net:50004, Object({"code": Number(1), "message": String("the transaction was rejected by network rules.\n\nTX decode failed\n[01000000000101458184848727618027be38febdf1203ebd57c44913a352ebb6106d1ae4a1c6b90000000000ffffffff0210270000000000001976a9141bdefe84a64de8f0929689dd38dc9b4e865bd2bd88acb6c21100000000001600141bdefe84a64de8f0929689dd38dc9b4e865bd2bd0247304402205a9a2cd79484077c0a85f2cba2a68cc6af01b98731133cabcc81827e2294557b022040eb5b92f885aa49a8a833b6e1006bc19b2689dbed68484797eb534709252bab0121036b88f059d24b2641c0c324e1e5f513b3623294e98e5b17db20c49df1d07d1aeb6f04d163]")})) }

Please answer following questions and attach requested info - it'll help to solve issue faster
[WIP]

[artemii235]

@shamardy Could you take a look at this, please?

[smk762]

I synced PPC locally and the hex from original comment returned

error code: -22
error message:
TX decode failed

[shamardy]

@SirSevenG Can you please test with this coin configuration?

  {
    "coin": "PPC-segwit",
    "name": "peercoin",
    "fname": "Peercoin",
    "txversion": 3,
    "rpcport": 9902,
    "pubtype": 55,
    "p2shtype": 117,
    "wiftype": 183,
    "decimals": 6,
    "txfee": 0,
    "dust": 10000,
    "segwit": true,
    "bech32_hrp": "pc",
    "address_format": {
      "format": "segwit"
    },
    "orderbook_ticker": "PPC",
    "mm2": 1,
    "required_confirmations": 1,
    "avg_blocktime": 8.5,
    "protocol": {
      "type": "UTXO"
    },
    "derivation_path": "m/44'/6'",
    "trezor_coin": "Peercoin",
    "links": {
      "github": "https://github.com/peercoin/peercoin",
      "homepage": "https://peercoin.net"
    }
  },

"isPoS": 1 is removed and "txversion": 3 is added instead of the default "txversion": 1

This is based on
https://github.com/peercoin/peercoin/blob/10ff5fb4ea5386ffc1df5c720da16553fe857724/src/primitives/transaction.h#L220,
https://github.com/peercoin/peercoin/blob/10ff5fb4ea5386ffc1df5c720da16553fe857724/src/primitives/transaction.h#L264
Where n_time is not deserialized/serialized and defaults to 0 in version 3.

[SirSevenG]

@SirSevenG Can you please test with this coin configuration?

@shamardy Thanks! Seems to work now. Will need to adjust "txfee": 0, tho, unless custom fee is specified now you'll get only "the transaction was rejected by network rules.\n\nbad-txns-fee-not-enough\n" errors.

It kinda worked before, and same 0 fee config works for non-segwit/legacy PPC. What is expected behaviour with "txfee": 0,?

[cipig]

"txfee": 0 means ask native wallet or electrum server for fee
https://electrumx-spesmilo.readthedocs.io/en/latest/protocol-methods.html#blockchain-estimatefee

[shamardy]

It kinda worked before, and same 0 fee config works for non-segwit/legacy PPC

This might be related to transaction weight/vsize calculations that's related to segwit. estimatefee returns the estimated fee per kb, the size of the transaction is calculated in mm2 to calculate the estimated fee for the transaction.
PPC seem to use the same segwit tx weight formula used in mm2 https://github.com/peercoin/peercoin/blob/10ff5fb4ea5386ffc1df5c720da16553fe857724/src/consensus/validation.h#L143-L146 so I can't find a problem in this.

I will test this myself by increasing the weight/vsize of the tx a bit and see which weight/vsize is accepted, they might have added other calculations elsewhere in the code that I am not aware of.

Also, peercoin block explorers don't show the right vsize for segwit transactions https://explorer.peercoin.net/tx/34ec9f2c1ae824f531eb00a040f9441c0067490b6ecef2ab51b768287ba328df , vsize is 222 here which is the size of the transaction not the virtual size, the virtual size according to my calculations should be 141. If it turns out that the actual size should be used for fee calculations in segwit, I wouldn't see the point in supporting segwit for PPC since fees are not reduced.

[shamardy]

I will test this myself by increasing the weight/vsize of the tx a bit and see which weight/vsize is accepted

I had to increase the tx size for fee calculations by 80 bytes for the transaction to be accepted (size used for fee estimation = 193 instead of 113), this is near the actual byte size for non-segwit transactions which is 225. This leads me to the conclusion that either vsize is not actually used in peercoin chain or estimatefee method returns a lower fee/kb than expected. I don't think we should change anything in mm2 to support this, what do you think @artemii235 ?
@cipig do you think we should use fixed fee for PPC-segwit instead?

[cipig]

i think that using fixed fee (per kb) is generally a good idea... we just need to find the correct values
there is usually a variable
src/wallet/wallet.h: static const CAmount DEFAULT_TRANSACTION_MINFEE = 100000;

looked in PPC code and can't find such a variable, instead i have this

src/consensus/amount.h:static constexpr CAmount CENT = 10000;
src/consensus/amount.h:static const CAmount MIN_TX_FEE_PREV7 = CENT;
src/consensus/amount.h:static const CAmount MIN_TX_FEE = CENT / 10;
src/consensus/amount.h:static const CAmount PERKB_TX_FEE = CENT;
src/consensus/amount.h:static const CAmount MIN_TXOUT_AMOUNT = CENT;

src/wallet/wallet.cpp:    CAmount nMinFeeBase = (IsProtocolV07(txNew.nTime) ? MIN_TX_FEE : MIN_TX_FEE_PREV7);

so loosk like txfee for PPC should be either 10000 or 1000, dending on the version... weird, but ok

using good fixed txfees per kbyte also spares us the estimatefee call and any problems associated with dynamic fees

electrums show this

(echo '{ "id": 1, "method": "blockchain.estimatefee", "params": ["1"] }'; sleep 0.5) | ncat --ssl electrum.peercoinexplorer.net 50002
{"jsonrpc": "2.0", "result": 0.01, "id": 1}

(echo '{ "id": 1, "method": "blockchain.estimatefee", "params": ["1"] }'; sleep 0.5) | ncat --ssl allingas.peercoinexplorer.net 50002
{"jsonrpc": "2.0", "result": 0.01, "id": 1}

same as wallet

peercoin-cli estimatesmartfee 1
{
  "feerate": 0.01,
  "blocks": 665357
}

and since PPC has only 6 decimals, that is 10000 sats... so should actually work with those values too

[shamardy]

I am leaning towards not supporting segwit for peercoin, these 3 transactions
https://explorer.peercoin.net/tx/69c27443fa9add1efffa0e3d354b694824c69a92b1fc24a826480c9796f85d4d
https://explorer.peercoin.net/tx/7c1136a0b33037b026cef64c02db5d6d613e1342a2c1e3dddd0db1e410e405f7
https://explorer.peercoin.net/tx/7e8ac36ac89187cefe922675c7ecbe60a8f7bd6c9a9d7b3bff1c262ae7f60116
are the 3 transactions I made from a segwit address.
I increased the transaction size and in return the fee for every transaction until it was accepted, for all 3 transactions it was only accepted when the actual bytes size was used not the vsize. It seems that segwit is not really supported for peercoin (no lower fees), only segwit address formats are supported and this has no benefit from legacy. You can check this by multiplying the size field in the raw transaction by the feerate which is 0.01 as @cipig mentioned here #1631 (comment). I also checked their web wallet and it uses legacy address formats with no segwit support.
As for legacy PPC, we can still use version 3 and remove isPOS field. This will reduce the transaction size a bit by removing n_time.

[cipig]

there is actually only one UTXO coin that needs segwit to save on txfees, BTC, for all others, the txfees are negligible, << 0.01 USD

[shamardy]

there is actually only one UTXO coin that needs segwit to save on txfees, BTC, for all others, the txfees are negligible, << 0.01 USD

I agree, but other UTXOs are easy to add and don't need code changes in mm2. For PPC we need to add a field in config to specify that segwit transaction size is calculated like regular transactions and make some changes in the code for this without any benefit for the user while their wallet (tried the web one only, not sure about the others) don't generate segwit addresses for the user.
Another thing @cipig, it seems that updating txversion for some coins can be beneficial, I can open an issue in coins repo to look for other coins that can benefit from this if you think this is a good idea. I already discussed this with @artemii235 in last dev meeting but want to hear your opinion on it.

[cipig]

So lets just remove PPC-segwit if it has no benefit.

Another thing @cipig, it seems that updating txversion for some coins can be beneficial, I can open an issue in coins repo to look for other coins that can benefit from this if you think this is a good idea. I already discussed this with @artemii235 in last dev meeting but want to hear your opinion on it.

Sure, sounds good. Let me know if you already have a list of potential candidates. I can do the test swaps with the new txversion and see if it's working.

[shamardy]

Sure, sounds good. Let me know if you already have a list of potential candidates. I can do the test swaps with the new txversion and see if it's working.

I opened an issue in the coins repo KomodoPlatform/coins#628.

So lets just remove PPC-segwit if it has no benefit.

@SirSevenG If we all agree on this, we can close this issue and remove PPC-segwit from the coins file/repo.