Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: discounted fees for confidential transactions #1317

Merged
merged 7 commits into from
May 14, 2024

Conversation

delta1
Copy link
Member

@delta1 delta1 commented Feb 9, 2024

ELIP 200: Discounted fees for Confidential Transactions

This is 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 410 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.

@delta1 delta1 force-pushed the ct-fees-discount branch 2 times, most recently from 2958009 to 946d62f Compare February 9, 2024 13:39
@tiero
Copy link

tiero commented Feb 9, 2024

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

@psgreco
Copy link
Contributor

psgreco commented Feb 9, 2024

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
Copy link
Member Author

delta1 commented Feb 10, 2024

yes this is optional and backwards compatible

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

@delta1 delta1 marked this pull request as draft February 13, 2024 14:11
@delta1 delta1 force-pushed the ct-fees-discount branch 2 times, most recently from 9b55936 to 8d9ba10 Compare February 22, 2024 15:52
@delta1 delta1 marked this pull request as ready for review February 22, 2024 15:52
@delta1
Copy link
Member Author

delta1 commented Feb 22, 2024

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

@delta1 delta1 force-pushed the ct-fees-discount branch from 8cf655c to 790af77 Compare March 20, 2024 13:54
@delta1
Copy link
Member Author

delta1 commented Mar 20, 2024

Force pushed to include commit which changes the block template ordering

@delta1 delta1 force-pushed the ct-fees-discount branch from 790af77 to 41abd40 Compare March 27, 2024 11:47
Copy link
Contributor

@roconnor-blockstream roconnor-blockstream left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My review is incomplete, but this is my first pass.

src/validation.cpp Outdated Show resolved Hide resolved
src/txmempool.h Outdated Show resolved Hide resolved
src/txmempool.h Outdated Show resolved Hide resolved
src/policy/discount.h Outdated Show resolved Hide resolved
src/txmempool.cpp Outdated Show resolved Hide resolved
src/txmempool.h Outdated Show resolved Hide resolved
@apoelstra
Copy link
Member

Done reviewing 41abd40

src/net_processing.cpp Show resolved Hide resolved
src/policy/discount.h Outdated Show resolved Hide resolved
src/txmempool.cpp Outdated Show resolved Hide resolved
src/txmempool.cpp Outdated Show resolved Hide resolved
src/node/miner.cpp Show resolved Hide resolved
@delta1 delta1 force-pushed the ct-fees-discount branch from 41abd40 to d0c18ed Compare April 12, 2024 13:50
@delta1
Copy link
Member Author

delta1 commented Apr 12, 2024

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

Copy link
Contributor

@roconnor-blockstream roconnor-blockstream left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am no longer able to find issues with this PR.

I didn't review the testing.

@delta1
Copy link
Member Author

delta1 commented May 6, 2024

"silent conversion between integer types of different signedness"

gotta love C++ :)

@delta1
Copy link
Member Author

delta1 commented May 7, 2024

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

@apoelstra
Copy link
Member

@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

@delta1 delta1 force-pushed the ct-fees-discount branch from 4fb3706 to 83a7fe5 Compare May 9, 2024 09:54
@apoelstra
Copy link
Member

utACK 83a7fe5

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
Copy link
Contributor

I have nothing to add.

@delta1 delta1 force-pushed the ct-fees-discount branch from 83a7fe5 to 3ebc353 Compare May 14, 2024 17:05
@delta1
Copy link
Member Author

delta1 commented May 14, 2024

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

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 3ebc353

@psgreco psgreco merged commit a4d7ac7 into ElementsProject:master May 14, 2024
8 of 13 checks passed
jamesdorfman added a commit to jamesdorfman/elements that referenced this pull request May 24, 2024
@delta1 delta1 deleted the ct-fees-discount branch May 28, 2024 12:56
apoelstra added a commit to ElementsProject/ELIPs that referenced this pull request Jun 22, 2024
33e5c4d ELIP-0200: Discounted fees for Confidential Transactions (Byron Hambly)

Pull request description:

  first pass at ELIP for discounted CT as implemented in ElementsProject/elements#1317 and alternate implementation in ElementsProject/rust-elements#204

ACKs for top commit:
  apoelstra:
    utACK 33e5c4d -- did not verify the test vectors

Tree-SHA512: 1c2d10a7e6ae0750e53e2364f4a55a767fa99d600bf5a9eaa04f3b06d906e618c6b9f0097ab829c651a3152cf5e0475109de045b8c9109892cea81844893a000
apoelstra added a commit to ElementsProject/rust-elements that referenced this pull request Jul 2, 2024
b7fc82e tx: add discount_weight and discount_vsize (Byron Hambly)

Pull request description:

  adds implementation of discount virtual size from ElementsProject/elements#1317

  same test vectors in ElementsProject/elements#1341

ACKs for top commit:
  apoelstra:
    ACK b7fc82e

Tree-SHA512: 48f0daafcb93bbcabacff4bb5cd5bc6b2dd99fb97117671f58d2b33c9b9e6357e01eb16d726dcbeb70375c6307b106a11d091604aafde148b5c8377f471e46f1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants