include the chaintip blockindex in the SyncTransaction signal, add signal UpdateTip() #6480

• allows reducing of calls to main.cpp for getting the chaintip during transaction syncing
• potentially allows reducing of cs_main locks

No objection, but I think it would make more sense to have an UpdatedTip signal in ValidationInterface which passed this just once, and let CWallets cache this value.

I think the SetBestChain() is already there for a such case (tip was updated). Having the CBlockIndex together with the CBlock would make sure we get the data together with the transaction to avoid deltas. And this solution has one big plus: the wallet(s) can detect if the transaction is in the mempool or not because the mempool SyncTransaction gives for CBlock as well as for CBlockIndex a NULL value where the disconnect block only leaves CBlock to null.

I think this could potentially decouple the wallet from the mempool.

SetBestChain is only called occasionally, and only when data is being flushed to disk.

Ah, I see, this passes the block index entry in which a transaction was found - which is not necessarily the current tip (for example during rescan).

Untested ACK.

I see the problem with SetBestChain()... So it would make sense to have an UpdateTip() signal next to the conditional variable (https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1995)?

@jonasschnelli IMHO, yes. Note that that code position always has cs_main held, so listeners to such an UpdateTip signal couldn't do much (but updating a cached variable with the lastest best block is fine and useful).

@sipa: just detected that an additional UpdateTip() signal is unnecessary, because with this PR a wallet could keep track of the chaintip over the SyncTranscation() signal (connectTip and disconnectTip sends both a syncTransaction with the current chaintip), which a wallet could therefore compare and store locally. I try to reduce the cs_main locking to a very minimum and later even to zero (interact over RPC/ZMQ). I need a way now to avoid deadlocking risks and therefore keep cs_main away from the wallet code.

@jonasschnelli So a wallet that does not have any new transactions coming in won't know what the current tip is? The wallet needs that information to compute confirmations. Right now, those checks require a call to main (CMerkleTx::GetDepthInMainChainINTERNAL) with associated cs_main lock. If we'd keep CBlockIndex pointers for the wallet transactions, and keep a copy of the tip (via an UpdateTip) in the wallet, all of that could become independent.

Also, it's probably better to issue such an UpdateTip signal in ActivateBestChain, near the uiInterface.NotifyBlockTip call (or replace it...). That has no locks held, and guarantees a stable tip (UpdateTip() may be called in the middle of reorganizations).

EDIT: Sorry @jonasschnelli I skipped the part between brackets. You're right of course - I was somehow assuming that SyncTransaction would only be called when there were matching wallet transactions.

Agreed. Added a commit which would add a UpdateTip() signal.

Needs rebase

Rebased. Removed the UpdateTip() signal because it was already added during the ZMQ integration.

@jonasschnelli Needs travis kicked again. (Unrelated error, it seems)

Kicked.

Seems there is an actual bug here.

Needs rebase

Rebased. I think this is useful because it allows to remove even more cs_main locks (example: it would be possible for the wallet to keep track of the wax's height, etc.).

Also adapted the zmq interface for the slightly changed signal. Travis is now happy.

Concept ACK.

But unless I missed something this PR doesn't actually do anything yet right? Why not save the commit to be part of a PR that actually takes advantage of this information?

But unless I missed something this PR doesn't actually do anything yet right?

It allows the wallet to keep track of the wtx heights independent of mempool and main (=step in wallet/core separation).

Why not save the commit to be part of a PR that actually takes advantage of this information?

I just try to avoid a big "this is a new wallet" PR. IMO all PRs that are understandable and have a benefit (maybe not directly as a feature) could go in in advance.

ok no objection

utACK

ACK 7d0bf0b