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.

Sign up for GitHub

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

Improved transactions getting logic. #18

Open
wants to merge 3 commits into
base: multiple-hd-accounts
Choose a base branch
from
Open

Improved transactions getting logic. #18

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cbb2209
Improved transactions getting logic.
sergeylappo Apr 9, 2018
4d72161
Improvement to increase peformance and comply with docu
sergeylappo Apr 9, 2018
4d285e0
Logic fix
sergeylappo Apr 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions core/src/main/java/org/bitcoinj/wallet/Wallet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1616,7 +1616,7 @@ public boolean isConsistent() {
public void isConsistentOrThrow() throws IllegalStateException {
lock.lock();
try {
Set<Transaction> transactions = getTransactions(true);
Collection<Transaction> transactions = this.transactions.values();
Copy link

Choose a reason for hiding this comment

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

This is safe because we use the same lock as in getTransactions() and we don't change the transactions.

This is performant, as we don't need the Set property. Only iteration.


Set<Sha256Hash> hashes = new HashSet<Sha256Hash>();
for (Transaction tx : transactions) {
Expand Down Expand Up @@ -1663,6 +1663,9 @@ boolean isTxConsistent(final Transaction tx, final boolean isSpent) {
log.error("isAvailableForSpending != spentBy");
return false;
}
if (!isActuallySpent) {
return !isSpent;
}
Copy link

Choose a reason for hiding this comment

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

Please, explain it to me.

if (!isActuallySpent) {
    // return isSpent == isActuallySpent;
    // return isSpent == false;
    return !isSpent;
}

ok, so far so good. I would prefer if (!isActuallySpent) {break;} though, for simplicity. The inconsistency is an exceptional case, so optimizing boolean expressions at the expense of readability is probably not worth it. Also isActuallySpent variable becomes obsolete if you write it like this:

    boolean isTxConsistent(final Transaction tx, final boolean isSpent) {
        for (TransactionOutput o : tx.getOutputs()) {
            if (o.isAvailableForSpending()) {
                if (o.isMineOrWatched(this)) {
                    return !isSpent;
                }
                if (o.getSpentBy() != null) {
                    log.error("isAvailableForSpending != spentBy");
                    return false;
                }
            } else {
                if (o.getSpentBy() == null) {
                    log.error("isAvailableForSpending != spentBy");
                    return false;
                }
            }
        }
        return isSpent;
    }

which makes me even more confused about what is going on here.

Something like

    boolean isTxConsistent(final Transaction tx, final boolean isSpent) {
        Predicate<? super TransactionOutput> isUnspentPredicate = new Predicate<TransactionOutput>() {
            @Override
            public boolean apply(TransactionOutput transactionOutput) {
                return transactionOutput.isAvailableForSpending() && transactionOutput.isMineOrWatched(Wallet.this);
            }
        };
        return isSpent == !Iterators.any(tx.getOutputs().iterator(), isUnspentPredicate);
    }

would be readable but what about the getSpentBy() check? Is it necessary?

} else {
if (o.getSpentBy() == null) {
log.error("isAvailableForSpending != spentBy");
Expand Down Expand Up @@ -1909,7 +1912,7 @@ private Set<Transaction> findDoubleSpendsAgainst(Transaction tx, Map<Sha256Hash,
* Adds to txSet all the txns in txPool spending outputs of txns in txSet,
* and all txns spending the outputs of those txns, recursively.
*/
void addTransactionsDependingOn(Set<Transaction> txSet, Set<Transaction> txPool) {
void addTransactionsDependingOn(Set<Transaction> txSet, Collection<Transaction> txPool) {
Map<Sha256Hash, Transaction> txQueue = new LinkedHashMap<Sha256Hash, Transaction>();
for (Transaction tx : txSet) {
txQueue.put(tx.getHash(), tx);
Expand Down Expand Up @@ -2052,7 +2055,7 @@ private void receive(Transaction tx, StoredBlock block, BlockChain.NewBlockType
// change its confidence to PENDING (Unless they are also spending other txns IN_CONFLICT).
// Consider dependency chains.
Set<Transaction> currentTxDependencies = Sets.newHashSet(tx);
addTransactionsDependingOn(currentTxDependencies, getTransactions(true));
addTransactionsDependingOn(currentTxDependencies, transactions.values());
currentTxDependencies.remove(tx);
List<Transaction> currentTxDependenciesSorted = sortTxnsByDependency(currentTxDependencies);
for (Transaction txDependency : currentTxDependenciesSorted) {
Expand Down Expand Up @@ -2187,8 +2190,7 @@ public void notifyNewBestBlock(StoredBlock block) throws VerificationException {
setLastBlockSeenTimeSecs(block.getHeader().getTimeSeconds());
// Notify all the BUILDING transactions of the new block.
// This is so that they can update their depth.
Set<Transaction> transactions = getTransactions(true);
for (Transaction tx : transactions) {
for (Transaction tx : transactions.values()) {
if (ignoreNextNewBlock.contains(tx.getHash())) {
// tx was already processed in receive() due to it appearing in this block, so we don't want to
// increment the tx confidence depth twice, it'd result in miscounting.
Expand Down Expand Up @@ -2511,7 +2513,7 @@ public boolean maybeCommitTx(Transaction tx) throws VerificationException {
log.info("->pending (IN_CONFLICT): {}", tx.getHashAsString());
addWalletTransaction(Pool.PENDING, tx);
doubleSpendPendingTxns.add(tx);
addTransactionsDependingOn(doubleSpendPendingTxns, getTransactions(true));
addTransactionsDependingOn(doubleSpendPendingTxns, transactions.values());
for (Transaction doubleSpendTx : doubleSpendPendingTxns) {
doubleSpendTx.getConfidence().setConfidenceType(ConfidenceType.IN_CONFLICT);
confidenceChanged.put(doubleSpendTx, TransactionConfidence.Listener.ChangeReason.TYPE);
Expand Down Expand Up @@ -2884,12 +2886,13 @@ public void run() {
public Set<Transaction> getTransactions(boolean includeDead) {
lock.lock();
try {
Set<Transaction> all = new HashSet<Transaction>();
if (includeDead) {
return new HashSet<>(transactions.values());
}
Set<Transaction> all = new HashSet<>();
all.addAll(unspent.values());
all.addAll(spent.values());
all.addAll(pending.values());
if (includeDead)
all.addAll(dead.values());
return all;
} finally {
lock.unlock();
Expand Down Expand Up @@ -2991,7 +2994,7 @@ public List<Transaction> getRecentTransactions(int numTransactions, boolean incl
if (numTransactions > size || numTransactions == 0) {
numTransactions = size;
}
ArrayList<Transaction> all = new ArrayList<Transaction>(getTransactions(includeDead));
ArrayList<Transaction> all = new ArrayList<Transaction>(transactions.values());
Copy link

Choose a reason for hiding this comment

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

includeDead is not always true.

// Order by update time.
Collections.sort(all, Transaction.SORT_TX_BY_UPDATE_TIME);
if (numTransactions == all.size()) {
Expand Down Expand Up @@ -4440,7 +4443,7 @@ public void reorganize(StoredBlock splitPoint, List<StoredBlock> oldBlocks, List
// Map block hash to transactions that appear in it. We ensure that the map values are sorted according
// to their relative position within those blocks.
ArrayListMultimap<Sha256Hash, TxOffsetPair> mapBlockTx = ArrayListMultimap.create();
for (Transaction tx : getTransactions(true)) {
for (Transaction tx : transactions.values()) {
Map<Sha256Hash, Integer> appearsIn = tx.getAppearsInHashes();
if (appearsIn == null) continue; // Pending.
for (Map.Entry<Sha256Hash, Integer> block : appearsIn.entrySet())
Expand Down