feat(coinjoin): improve asset lock handling and other fixes #244

HashEngineering · 2024-01-10T19:24:19Z

Issue being fixed or feature implemented

What was done?

Fixed two bugs related to creating denominations
added transaction history report to wallet dump

How Has This Been Tested?

Breaking Changes

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation

For repository code-owners and collaborators only

I have assigned this pull request to a milestone

* refactor: rename credit funding to asset lock * tests: update tests

codecov · 2024-01-10T19:30:05Z

Codecov Report

Attention: 75 lines in your changes are missing coverage. Please review.

Comparison is base (eb4e227) 48.47% compared to head (bdae05b) 48.60%.

Files	Patch %	Lines
...a/org/bitcoinj/evolution/AssetLockTransaction.java	68.75%	23 Missing and 2 partials ⚠️
...t/authentication/AuthenticationGroupExtension.java	9.52%	18 Missing and 1 partial ⚠️
core/src/main/java/org/bitcoinj/wallet/Wallet.java	17.64%	13 Missing and 1 partial ⚠️
...re/src/main/java/org/bitcoinj/wallet/WalletEx.java	79.31%	3 Missing and 3 partials ⚠️
...e/src/main/java/org/bitcoinj/core/Transaction.java	58.33%	1 Missing and 4 partials ⚠️
...src/main/java/org/bitcoinj/wallet/SendRequest.java	50.00%	2 Missing ⚠️
...a/org/bitcoinj/coinjoin/CoinJoinClientSession.java	0.00%	1 Missing ⚠️
...ava/org/bitcoinj/coinjoin/CoinJoinSendRequest.java	0.00%	1 Missing ⚠️
.../java/org/bitcoinj/evolution/AssetLockPayload.java	93.75%	0 Missing and 1 partial ⚠️
...c/main/java/org/bitcoinj/script/ScriptPattern.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                  Coverage Diff                   @@
##             feature-coinjoin     #244      +/-   ##
======================================================
+ Coverage               48.47%   48.60%   +0.12%     
- Complexity               7446     7474      +28     
======================================================
  Files                     451      451              
  Lines                   50127    50169      +42     
  Branches                 7128     7128              
======================================================
+ Hits                    24300    24384      +84     
+ Misses                  23087    23043      -44     
- Partials                 2740     2742       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

core/src/main/java/org/bitcoinj/wallet/WalletEx.java

HashEngineering · 2024-01-10T20:04:17Z

core/src/main/java/org/bitcoinj/wallet/WalletEx.java

+    public String getTransactionReport() {
+        MonetaryFormat format = MonetaryFormat.BTC.noCode();
+        StringBuilder s = new StringBuilder("Transaction History Report");
+        s.append("\n-----------------------------------------------\n");
+


This function will give us a high level view of transaction history that will display transactions in order by date.

HashEngineering · 2024-01-10T20:05:00Z

core/src/main/java/org/bitcoinj/evolution/AssetLockPayload.java

    public String toString() {
-        return String.format("AssetLockPayload(creditOutputs: %d)",
-                creditOutputs.size());
+        StringBuilder s = new StringBuilder("AssetLockPayload");
+        creditOutputs.forEach(output -> {
+            Script scriptPubKey = output.getScriptPubKey();
+            s.append("\n   out  ");
+            s.append(scriptPubKey.getChunks().size() > 0 ? scriptPubKey.toString() : "<no scriptPubKey>");
+            s.append("  ");
+            s.append(output.getValue().toFriendlyString());
+            s.append('\n');
+            s.append("        ");
+            Script.ScriptType scriptType = scriptPubKey.getScriptType();
+            s.append(scriptType).append(" addr:").append(scriptPubKey.getToAddress(params));
+        });
+        return s.toString();
    }


This output will be similar to that of Transaction.toString

HashEngineering · 2024-01-10T20:06:16Z

core/src/main/java/org/bitcoinj/script/ScriptBuilder.java

+    public static Script createAssetLockOutput() {
        ScriptBuilder builder = new ScriptBuilder();
        builder.addChunk(new ScriptChunk(OP_RETURN, null));
-        builder.data(creditBurnKey.getPubKeyHash());
+        builder.data(new byte[0]);
        return builder.build();
    }


The OP_RETURN will not have a reference to the key that will sign the state transitions that uses this asset lock. instead this information is in the AssetLockPayload.

HashEngineering · 2024-01-10T20:06:55Z

core/src/main/java/org/bitcoinj/core/Transaction.java

+        if (version != MIN_STANDARD_VERSION) {
+            if (getVersionShort() == SPECIAL_VERSION) {
+                s.append(indent).append("version: ").append(getVersionShort()).append('\n');
+                Type type = (getVersionShort() == SPECIAL_VERSION) ? getType() : Type.TRANSACTION_NORMAL;
+                s.append("   type: ").append(type.toString()).append('(').append(type.getValue()).append(")\n");
+            } else {
+                s.append(indent).append("version: ").append(version).append('\n');
+            }
+        }


This is to make the version number nicer to look at. This should also be added to master.

core/src/main/java/org/bitcoinj/evolution/AssetLockTransaction.java

* fixes bug that prevented creation of more denominations past the goal count

HashEngineering · 2024-01-12T17:13:31Z

core/src/main/java/org/bitcoinj/wallet/WalletEx.java

+        for (TransactionOutput output : myUnspents) {
+            TransactionConfidence confidence = output.getParentTransaction().getConfidence();
+            // confirmations must be 0 or higher, not conflicted or dead
+            if (confidence != null && (confidence.getConfidenceType() == TransactionConfidence.ConfidenceType.PENDING || confidence.getConfidenceType() == TransactionConfidence.ConfidenceType.BUILDING)) {
+                // inputValue must match, the TX is mine and is not spent
+                if (output.getValue().equals(inputValue) && output.getSpentBy() == null) {
                    count++;
+                }
            }


This fixes a minor bug where spent outputs were counted. Over time this would keep some denominations from being unused, especially 0.001 and 0.01. Though there was another bug that was contributing to this too.

Also refactored to turn change this from O(n^2) to O(n).

Though the output.getSpentBy() == null is redundant because myUnspents would only have unspent outputs.

HashEngineering · 2024-01-12T17:15:03Z

core/src/main/java/org/bitcoinj/coinjoin/CoinJoinClientSession.java

-                    if (balanceToDenominate.isGreaterThanOrEqualTo(Coin.ZERO)) break;
+                    if (balanceToDenominate.isLessThanOrEqualTo(Coin.ZERO)) break;


This problem of > being used instead of < meant that this for loop was not being executed when it should have been. This bug prevented part of the demomination process from running.

Syn-McJ

Looks good

HashEngineering added 4 commits January 10, 2024 11:21

feat: improve asset lock handling to match v20 updates

5c6e400

* refactor: rename credit funding to asset lock * tests: update tests

fix(coinjoin): add missing is spent check to countInputsWithAmount

b68e33d

feat(wallet-tool): add parameter to return change with coinjoin send

2b39087

feat(coinjoin): add tx history at the end of a wallet dump

6b51275

HashEngineering self-assigned this Jan 10, 2024

HashEngineering mentioned this pull request Jan 10, 2024

feat(dashpay): use v20 assetlocks dashpay/dash-wallet#1246

Merged

3 tasks

HashEngineering changed the base branch from master to feature-coinjoin January 10, 2024 19:33

HashEngineering commented Jan 10, 2024

View reviewed changes

core/src/main/java/org/bitcoinj/wallet/WalletEx.java Outdated Show resolved Hide resolved

HashEngineering commented Jan 10, 2024

View reviewed changes

core/src/main/java/org/bitcoinj/evolution/AssetLockTransaction.java Outdated Show resolved Hide resolved

HashEngineering added 6 commits January 10, 2024 13:21

refactor: improve AssetLockTransaction for multiple credit outputs

7442e57

refactor: rename setAssetLockPublicKey to addAssetLockPublicKey

9dafbb0

refactor: rename setAssetLockPublicKey to addAssetLockPublicKey

12275a0

tests: add more tests to AuthenticationGroupExtensionTest

5bec0c6

fix: fix incorrect inequality check in createDenominate

e936c85

* fixes bug that prevented creation of more denominations past the goal count

feat: add txid to the transaction history report

bdae05b

HashEngineering commented Jan 12, 2024

View reviewed changes

Syn-McJ approved these changes Jan 16, 2024

View reviewed changes

HashEngineering marked this pull request as ready for review January 16, 2024 21:52

HashEngineering merged commit f6cd23d into feature-coinjoin Jan 16, 2024
4 of 6 checks passed

HashEngineering deleted the feature-coinjoin-assetlocks branch March 19, 2024 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(coinjoin): improve asset lock handling and other fixes #244

feat(coinjoin): improve asset lock handling and other fixes #244

HashEngineering commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 10, 2024 •

edited

Loading

HashEngineering Jan 10, 2024

HashEngineering Jan 10, 2024

HashEngineering Jan 10, 2024

HashEngineering Jan 10, 2024

HashEngineering Jan 12, 2024 •

edited

Loading

HashEngineering Jan 12, 2024 •

edited

Loading

Syn-McJ left a comment

		if (balanceToDenominate.isGreaterThanOrEqualTo(Coin.ZERO)) break;
		if (balanceToDenominate.isLessThanOrEqualTo(Coin.ZERO)) break;

feat(coinjoin): improve asset lock handling and other fixes #244

feat(coinjoin): improve asset lock handling and other fixes #244

Conversation

HashEngineering commented Jan 10, 2024 • edited Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

codecov bot commented Jan 10, 2024 • edited Loading

Codecov Report

HashEngineering Jan 10, 2024

Choose a reason for hiding this comment

HashEngineering Jan 10, 2024

Choose a reason for hiding this comment

HashEngineering Jan 10, 2024

Choose a reason for hiding this comment

HashEngineering Jan 10, 2024

Choose a reason for hiding this comment

HashEngineering Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

HashEngineering Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Syn-McJ left a comment

Choose a reason for hiding this comment

HashEngineering commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 10, 2024 •

edited

Loading

HashEngineering Jan 12, 2024 •

edited

Loading

HashEngineering Jan 12, 2024 •

edited

Loading