feat: discounted fees for confidential transactions

[delta1]

This is the first PR for a mempool policy only change to enable discounted fees for Confidential Transactions.

Two new configuration options are added to toggle accepting and creating these discounted CTs. accept defaults to true for liquidv1 only, false for other chains. create defaults to false on all chains for now.

At a feerate of 1 sat/vb for a 2 input, 3 output transaction:

• explicit transaction has a fee of 326 sats
• confidential transaction has a fee of 408 sats (without discount: 2575 sats)

This change is optional and backwards compatible for nodes and wallets.

This PR also alters block template transaction ordering, such that transactions are ordered by their feerate according to discountvsize. This means that discounted CT won't be selected last due to their lower real feerate according to vsize.

[tiero]

Please can you clearly add to the description if this change impacts wallets and other Elements implementation?

[psgreco]

Please can you clearly add to the description if this change impacts wallets and other Elements implementation?

The idea for this change is that it will be optional, and more beneficial to users, so if wallets keep doing the same thing they were doing, it will still be valid, they will just be overpaying a bit based on the new standard.

[delta1]

yes this is optional and backwards compatible

spec is ELIP 200: https://github.com/ElementsProject/ELIPs/blob/main/elip-0200.mediawiki

[delta1]

Force pushed with the change to calculate the discount per confidential output

[delta1]

Force pushed to include commit which changes the block template ordering

[apoelstra]

Done reviewing 41abd40b5cdbc910cabac009e4e27e670b2e502a

[delta1]

@roconnor-blockstream @apoelstra thanks for your reviews, addressed your comments

[apoelstra]

Done reviewing 7fc96512ff71c5fea9fb167403c64ac42af1070b. Just a couple minor things: would be good to change the RPC to not output discountvsize when acceptdiscounttxes is off, and would be good to change the discount computation to individually treat the output fields.

The rest are nits.

Fine if you just add commits, since that's what's been happening with this PR so far, but in general I would prefer if you were rebasing.

[apoelstra]

utACK 523c98d69ff416ec8e61a878b477b9f2c37171f8

[apoelstra]

In 3a4543986a74ab69fc464c054fc2871e2ae6fa11:

You still have the nAsset and nValue discounts conditioned on the witness being present. Why?

[apoelstra]

So, I guess there are three cases for the witness field:

• It is literally not present (length 0)
• It is present but unpopulated (length 2, just two length-prefix bytes for the range and surjection proof)
• It is present and populated (length > 2).

What conditions separate the first and second cases?

[delta1]

In 3a45439:

You still have the nAsset and nValue discounts conditioned on the witness being present. Why?

oversight, fixed

So, I guess there are three cases for the witness field:

It is literally not present (length 0)
It is present but unpopulated (length 2, just two length-prefix bytes for the range and surjection proof)
It is present and populated (length > 2).

What conditions separate the first and second cases?

From what I understand case 1 does not occur, since even explicit transactions have the single 0 byte for each proof

[apoelstra]

Your assertion is >= 0 but it should be >= 2.

[delta1]

Your assertion is >= 0 but it should be >= 2.

the 2 is subtracted in the line above

[apoelstra]

the 2 is subtracted in the line above

:grimacing: GetSerializeSize returns a size_t. I see now that you're implicitly casting it to a in64_t, which I suspect that the linter is failing on and is causing one of the red Xs in CI. This really makes me nervous. I don't like signed numbers, I don't like conversions from unsigned to (possibly differently sized) signed numbers in C++, and I don't like "sizes" that might be negative.

[roconnor-blockstream]

the 2 is subtracted in the line above

😬 GetSerializeSize returns a size_t. I see now that you're implicitly casting it to a in64_t, which I suspect that the linter is failing on and is causing one of the red Xs in CI. This really makes me nervous. I don't like signed numbers, I don't like conversions from unsigned to (possibly differently sized) signed numbers in C++, and I don't like "sizes" that might be negative.

When I was reviewing this, I noted that in upstream Bitcoin Core this all gets fixed up. So I figured eventually a rebase will fix this stuff.

That said, we should probably pass CI.

[delta1]

@apoelstra thanks for your feedback, updated. Initially had it as a size_t but the compiler was giving me a warning "comparison of unsigned expression in '>= 0' is always true". Changed now do the assert >= 2 before doing the subtraction.

@roconnor-blockstream there are a couple of CI failures fixed by #1330

[apoelstra]

Great, thanks! 4fb3706e6c6afc631a447920e6ec89c78ae1f310 looks good to me.

@apoelstra thanks for your feedback, updated. Initially had it as a size_t but the compiler was giving me a warning "comparison of unsigned expression in '>= 0' is always true". Changed now do the assert >= 2 before doing the subtraction.

Yes, but your initial change was little help to a human reviewer who expects a size to be a size_t :) And it replaced a "always true comparison" with a "silent conversion between integer types of different signedness". Which for some reason, gcc does not warn about.

[delta1]

"silent conversion between integer types of different signedness"

gotta love C++ :)

[delta1]

@apoelstra can I squash this into 1 commit and rebase on master?

[apoelstra]

@delta1 yeah, that'd be good. Though I would prefer if you could squash this into four or five commits. For example

• one that introduces the new config args, but don't use them to do anything
• one that introduces the new discount functions but doesn't use it
• one that hooks it up to the mempool logic
• one that hooks it up to the network logic
• one that hooks it up to the wallet logic

[apoelstra]

utACK 83a7fe5442d9af612183b20d988d367a74ce11de

I can't test this locally because, even on master, even with -j1, two functional tests are failing in my Nix environment (interface_bitcoin_cli --descriptors and mempool_persist). But I confirmed that everything appears to build on the tip of this PR, and the code changes look good (and the total diff (nearly) matches the previous 20-commit thing).

[roconnor-blockstream]

I have nothing to add.

[delta1]

pushed to update liquidv1 default for accept_discount_ct to false

git range-diff 7ff33d45df3a07b421cd7bacc63e9f34fef72fcb..3ebc35359f1304ff9f3a2a5286a154287794e23f 7ff33d45df3a07b421cd7bacc63e9f34fef72fcb..83a7fe5442d9af612183b20d988d367a74ce11de
1:  c1661b251a ! 1:  c2692d9907 discount: introduce config args
    @@ src/chainparams.cpp: public:
      
              multi_data_permitted = true;
     +        create_discount_ct = args.GetBoolArg("-creatediscountct", false);
    -+        accept_discount_ct = args.GetBoolArg("-acceptdiscountct", false);
    ++        accept_discount_ct = args.GetBoolArg("-acceptdiscountct", true);
      
              parentGenesisBlockHash = uint256S("000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f");
              const bool parent_genesis_is_null = parentGenesisBlockHash == uint256();
2:  9cbd3e0f05 = 2:  283d245bd9 discount: add policy/discount.h
3:  b7b2288a74 = 3:  76d42f5056 discount: implement mempool logic
4:  c85f0d4fda = 4:  146b1acc71 discount: implement net processing
5:  4807f3ef1f = 5:  c7dae295ba discount: change miner tx ordering
6:  52962c16c9 = 6:  b30b79d653 discount: allow wallet to create discount txs
7:  3ebc35359f = 7:  83a7fe5442 discount: add functional tests