Fix wallet sync for RpcBlockchain

[evanlinjin]

Fixes #677

Description

Unfortunately to fix all the problems, I had to do a complete re-implementation of RpcBlockchain.

The new implementation fixes the following:

• We can track more than 100 scriptPubKeys
• We can obtain more than 1000 transactions per sync
• Transaction "metadata" for already-syned transactions are updated when we introduce new scriptPubKeys

RpcConfig changes:

• Introduce RpcSyncParams.
• Remove RpcConfig::skip_blocks (this is replaced by RpcSyncParams::start_time).

Notes to the reviewers

• The RpcConfig structure is changed. It will be good to confirm whether this is an okay change.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

* [ ] I've added tests for the new feature

• I've added docs for the new feature
• I've updated CHANGELOG.md

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[evanlinjin]

We should probably check whether this fixes #598

[danielabrozzoni]

This review was a collaborative effort between me and @afilini, but I want to make it clear that if some comments are stupid, it's only his fault. (jk) (well, maybe not)

Concept ACK, some comments

[danielabrozzoni]

you can use db_spk_count

[danielabrozzoni]

Also, maybe you should check those per KeychainKind: start_script_count = 100 imho should mean 100 cached for the external and 100 cached for the internal descriptor. Otherwise you risk having start_script_count = 100 and, instead of 50/50, having 100 cached for the external and 0 for the internal.

If that's the case, maybe we should rename count -> index? So you can say "index = 50" instead of count = 100 and it's clearer

[evanlinjin]

Good catch. Changed.

        let ext_spks = db.iter_script_pubkeys(Some(KeychainKind::External))?;
        let int_spks = db.iter_script_pubkeys(Some(KeychainKind::Internal))?;

        // This is a hack to see whether atleast one of the keychains comes from a derivable
        // descriptor. We assume that non-derivable descriptors always has a script count of 1.
        let last_count = std::cmp::max(ext_spks.len(), int_spks.len());
        let has_derivable = last_count > 1;

        // If atleast one descriptor is derivable, we need to ensure scriptPubKeys are sufficiently
        // cached.
        if has_derivable && last_count < params.start_script_count {
            let inner_err = MissingCachedScripts {
                last_count,
                missing_count: params.start_script_count - last_count,
            };
            debug!("requesting more spks with: {:?}", inner_err);
            return Err(Error::MissingCachedScripts(inner_err));
        }

[danielabrozzoni]

maybe here it's better for readability if you use match?

[evanlinjin]

I feel like it's a matter of taste, but I've change it :/

            .filter_map(|keychain| match db.get_last_index(*keychain) {
                Ok(li_opt) => li_opt.map(|li| Ok((*keychain, li))),
                Err(err) => Some(Err(err)),
            })

[danielabrozzoni]

It's a bit unclear that in the None case, you're inserting in the same thing you're matching on (took me a second). What about:

                match &mut db_tx.transaction {
                    Some(raw_tx) => raw_tx,
                    tx_opt => {
                        updated = true;
                        tx_opt.insert(client.get_raw_transaction(
                            &tx_res.info.txid,
                            tx_res.info.blockhash.as_ref(),
                        )?)
                    }

[evanlinjin]

Great idea. Changed.

[danielabrozzoni]

Same as above, a match here might be more readable

[evanlinjin]

Changed.

[danielabrozzoni]

You might not need to collect into a Vec, as you're iterating them just below

[danielabrozzoni]

OTOH... Why do you need two loops? Can't you record the updates directly when you found them?

[evanlinjin]

It's because we cannot have two mutable references (self is already a mutable reference).

The solution I changed to is to clone self.txs first. However, I am thinking now DbState::update_state should really returns DbStateUpdates instead of having self mutable? But I am already spending too much time on this PR!!! haha

[danielabrozzoni]

I'm not sure if it's worth it to have both a last_indexes and a updated_last_indexes, as it's quite complex and not a great performance improvement. You could simply have last_indexes and at the end always rewrite the last index of each keychain in the database - without really caring if you didn't modify it. It's just two rows to update, not thousands, and I feel like it would make the code significantly easier to read.

[evanlinjin]

Removed!

[danielabrozzoni]

I don't think this will work, as usually batch.del_tx will return Some only if you're deleting something that has just been inserted in the batch.

[evanlinjin]

Fixed.

[danielabrozzoni]

Is it? You're still doing:

if batch.del_tx(txid, false)?.is_some() {
    debug!("deleting tx: {}", txid);
    del_txs += 1;
}

But my point is that del_tx will delete a transaction even when returning None...

Ping @afilini for an explanation, I don't remember why is the BatchDatabase doing so

[afilini]

It's because the batch object is basically disconnected from the actual database, it's just a collection of operations that are then applied at once at the end

[evanlinjin]

@afilini @danielabrozzoni Thanks for pointing this out! I'll actually fix it now

[danielabrozzoni]

Is this count correct? Above, you filter_map the updated_txs before inserting them

[evanlinjin]

I only filter the deleted txs, in which case I have the del_txs variable :)

[danielabrozzoni]

Maybe if self.txs.get(txid) never returns None it's better to expect with an explanation?

....Unless there are some cases where None might be returned! But I can't think of any

[evanlinjin]

Sorted.

        // update txs
        self.updated_txs
            .iter()
            .inspect(|&txid| debug!("updating tx: {}", txid))
            .try_for_each(|txid| batch.set_tx(self.txs.get(txid).unwrap()))?;

[afilini]

I just have a few very small comment, code looks very good now!

[afilini]

nit: you can use self.db and remove the argument

[evanlinjin]

Looking at how received_from_raw_tx is used, it is used while a mutable reference to a field of self still exists.

The suggested change will result in:

error[E0502]: cannot borrow `*self` as immutable because it is also borrowed as mutable

[afilini]

nit: at least

[evanlinjin]

English is such an annoying language. Rules do not make sense. Anyway, thank you for pointing this out! haha

[afilini]

nit: more than one (or alternatively just "two")

[afilini]

I have a couple of suggestions here to improve this a bit:

1. instead of cloning the whole thing you can call values() first, so at least you avoid cloning the keys
2. the check that looks for the tx in retained_txs could be moved into a filter() method, which maybe makes this a bit more readable. Also this check could be moved before the clone() to again avoid cloning things you don't need

[evanlinjin]

@afilini Thank you for the suggestions!

However, using .values().cloned() would not work, as calling .cloned() only clones one element at a time (and we have a mutable reference of the whole self).

In my original commit, I had all filters in an .filter_map and had two loop (one for marking changes and one for applying changes) so we avoid the mutable-reference problems.

I think it is best to kinda go back to my original solution, but improve commenting and split "filter logic" into multiple .filter..() calls.

This is what I have now:

        // obtain vector of `TransactionDetails::sent` changes
        let sent_updates = self
            .txs
            .values()
            // only bother to update txs that are retained
            .filter(|db_tx| self.retained_txs.contains(&db_tx.txid))
            // only bother to update txs where the raw tx is accessable
            .filter_map(|db_tx| (db_tx.transaction.as_ref().map(|tx| (tx, db_tx.sent))))
            // recalcuate sent value, only update txs in which sent value is changed
            .filter_map(|(raw_tx, old_sent)| {
                self.sent_from_raw_tx(raw_tx)
                    .map(|sent| {
                        if sent != old_sent {
                            Some((raw_tx.txid(), sent))
                        } else {
                            None
                        }
                    })
                    .transpose()
            })
            .collect::<Result<Vec<_>, _>>()?;

        // record send updates
        sent_updates.iter().for_each(|&(txid, sent)| {
            // apply sent field changes
            self.txs.entry(txid).and_modify(|db_tx| db_tx.sent = sent);
            // mark tx as modified
            self.updated_txs.insert(txid);
        });

This was my original:

bdk/src/blockchain/rpc.rs

Lines 454 to 476 in 9d787bf

	// update sent from tx inputs
	let sent_updates = self
	.txs
	.values()
	.filter_map(|db_tx| {
	let txid = self.retained_txs.get(&db_tx.txid)?;
	self._sent_from_raw_tx(db, db_tx.transaction.as_ref()?)
	.map(|sent| {
	if db_tx.sent != sent {
	Some((*txid, sent))
	} else {
	None
	}
	})
	.transpose()
	})
	.collect::<Result<Vec<_>, _>>()?;
	// record send updates
	sent_updates.into_iter().for_each(|(txid, sent)| {
	self.txs.entry(txid).and_modify(|db_tx| db_tx.sent = sent);
	self.updated_txs.insert(txid);
	});

[evanlinjin]

I just had a thought. If we can guarantee that txs returned from listtransactions RPC call are in chronological order (which I assume it would be), we can have the TransactionDetails::sent recalculations in the same loop as the recalculations of all the other fields.

The only benefit of this is for a slight performance gain, so maybe no point to bother? (I would assume that reducing IO would contribute the most to performance)

[afilini]

Yeah I remember this now, I think I was the one who suggested merging the two loops 😅

In the end I like what you came up with!

[afilini]

Also if you could rebase to fix the conflicts in changelog.md

[afilini]

ACK 5eeba6c