CHub Rev. 3 #1408

TheBlueMatt commented at 1:50 AM on June 1, 2012: member

Due to several changes in master that would require rethinking decisions made in the second revision of CBlockStore (#771) and thus very large code changes to rebase, I decided it was better to redo and manually rebase and re-commit the parts that remain relevant, also decreasing the total size of the changes to just those required to get CHub working on master. Some of the more disturbing changes can be added later after merge.

This is early in the process, but I thought I'd get it in the queue and let people review the commits as they come in and give feedback, if they wish.

in src/blockstore.h:None in 8ee6972889 outdated

   0 | @@ -0,0 +1,99 @@
   1 | +#ifndef BITCOIN_BLOCKSTORE_H
   2 | +#define BITCOIN_BLOCKSTORE_H
   3 | +
   4 | +// This API is considered stable ONLY for existing bitcoin codebases,
   5 | +// any futher uses are not yet supported.
   6 | +// This API is subject to change dramatically overnight, do not
   7 | +// depend on it for anything.
   8 | +

gavinandresen commented at 12:27 PM on June 1, 2012:

What is the big-picture purpose of CBlockStore ? Is it for storing blocks in the database? Being told about new blocks and keeping track of the best chain? Both? Something else?

And what's up with ProcessCallbacks-- I'm not seeing why that's necessary, or why BlockStore would expose threads, but maybe that's because I don't understand the big-picture purpose/design.

TheBlueMatt commented at 12:33 PM on June 1, 2012:

The purpose of CBlockStore is to act as a hub (oops, Im gonna rename it to CHub) between p2p, wallet and the block storage. The block storage (including being told about new blocks, holding the chain (or not, if SPV)) will be hidden behind CHub, as seen by p2p and wallet. A CHub can then callback to a CBlockStore/CSPVBlockStore/etc. Eventually, you could even run p2p and wallet clients in separate processes for additional security, that is the long-long-long term goal. In terms of exposing threads, that is more for added functionality later which cleans up the API a bit (when you need to run n regular threads, and possibly multiple callback threads).

TheBlueMatt commented at 2:43 AM on June 2, 2012: member

Updated with all the feedback so far. Any further work (aside from bug-fixes) will be done on other branches, so this branch is ready for review. Take special note of the last commit (and its very long commit message).

Add a CHub for communication from p2p/wallet to blockstore.

The goal is for p2p code/wallet to only get information/communicate
information about the blockchain through CHub, giving Bitcoin a
much more clearly-defined structure and allowing for the removal
of a ton of the current global mess.

877e3af8ba

Add a basic CHubListener that can be extended. c42d0f15a2

Add EmitBlock/CommitBlock functionality to CHub.

Replacing ProcessBlock with EmitBlock, and creating callbacks for
CommitBlock.

dd3593c500

Fix typo e6d39a8873

Add HandleCommitBlock to net.cpp & move stuff from main.cpp to it 159c528cec

Remove uiInterface.NotifyBlocksChanged and replace with CommitBlock

Also rename NotifyBlocksChanged to NotifyNewBlock and remove from
the uiInterface signals list.

This removes some functionality, but NofiyBlocksChanged was not
used anyway, so it shouldn't matter.  That said, if it is ever
needed, it would be fairly trivial to add a new callback for it
in CHub.

94a440ae79

Add EmitAlert/CommitAlert functionality to CHub.

Replace ProcessAlert with calls to EmitAlert and create callbacks
for CommitAlert.

1aad2e99bb

Use CommitAlert in qt/clientmodel.cpp 5365eca78c

Add RegisterRemoveAlert functionality to CHub. 7da0bed1f2

Replace NotifyAlertChanged with RegisterRemoveAlert. 8f6c206561

Convert Orphan Tx storage to CTransactions from CDataStreams.

There was no reason to use CDataStream as the transaction was
already being serialized/deserialized several times, with this
change, transactions coming in over network are deserialized once
when received, and then only reserialized in the call to
RelayMessage, which will be called in a callback thread, not
blocking cs_main.

c6c30dca1a

Add a cs around mapAlreadyAskedFor. 9dc89066a8

Add basic EmitTransaction/CommitTransaction functionality to CHub. ac33eeca2c

Use EmitTransaction instead of AcceptToMemoryPool in sendrawtx. 3bfb60f64d

Add CWallet support for registering with a CHub. c277c93f23

Use HandleCommitTransactionToMemoryPool instead of SyncWithWallets. 916ffbfe9d

Remove AcceptToMemoryPool and replace with EmitTransaction.

 * This removes not only CTransaction::AcceptToMemoryPool, but
    also CMerkleTx::AcceptToMemoryPool. It also moves
    CWalletTx::AcceptWalletTransaction to wallet.cpp

 * This adds a fCheckInputs flag to EmitTransaction, which is
    similar to the fCheckInputs flag to AcceptToMemoryPool,
    however, it has stricter guidlines that it should only be set
    "when transaction is a supporting tx for one of our own."
    Additionally, "fCheckInputs is ignored (and set to true)
    if !IsInitialBlockDownload() && !fClient"

    As a part of these guidelines,
    CWalletTx::AcceptWalletTransaction calls EmitTransaction with
    fCheckInputs set to true (the default) on the final
    transaction, whereas it used to call with fCheckInputs set to
    false. This has the important side-effect of allowing wallet-
    generated transactions to end up getting AddOrphanTx'd.
    However, if a supporting transaction to one of our own had
    previously been AddOrphanTx'd, it would immediately be added
    to memory pool as it is "a supporting tx for one of our own"
    and thus is re-added with fCheckInputs=false.

    Note that the possibility of a wallet transaction getting
    AddOrphanTx'd is very low, and should only happen if
    a) a transaction's input is a generate and we are missing that
       block (note that no transactions should be generated with a
       generation input if we don't have that block anyway).
    b) We match the !IsInitialBlockDownload() && !fClient check,
       are not caught up to the latest block, and an input is in a
       block we do not yet have (possible after the last
       checkpoint). This situation is temporary and should resolve
       itself once we catch up (though AddOrphanTx'd transactions
       may be permanently orphaned).

    Largely, these guidelines are there because there is no reason
    to add a transaction without checking its inputs, as we have
    those inputs available, and checking them as any other
    transaction would provides additional sanity-checks.

 * A second EmitTransaction was added with tx of type CMerkleTx.
    This keeps behavior of CMerkleTx::AcceptToMemoryPool the same
    in fClient mode. Note that new behavior was invented for
    CHub::EmitTransaction(CTransaction&...) in fClient mode,
    namely that ClientConnectInputs is only checked if
    fCheckInputs is true. This was chosen to make emitting a
    transaction possible in fClient mode even if its inputs are
    not available, but could be changed if support for that is not
    needed when fClient mode is actually implemented.

e60084cae3

in src/wallet.h:None in b8fe0a0262 outdated

  57 | @@ -57,7 +58,7 @@ class CKeyPool
  58 |  /** A CWallet is an extension of a keystore, which also maintains a set of transactions and balances,
  59 |   * and provides the ability to create new transactions.
  60 |   */
  61 | -class CWallet : public CCryptoKeyStore
  62 | +class CWallet : public CCryptoKeyStore, public CHubListener

sipa commented at 2:47 PM on June 8, 2012:

I suppose CHubListener could be derived protected. No class except CWallet should be able to call RegisterWithHub and related functions. The callback functions in CHubListener can be made protected directly I suppose - they are only intended to be overridden, not to be called by anyone except via CHub's signals.

TheBlueMatt commented at 8:34 PM on June 10, 2012:

Currently pwalletMain->RegisterWithHub(phub) is called from init.cpp, and I kinda like that design. That said, I did go ahead and make the callback virtual functions protected.

in src/hub.h:None in b8fe0a0262 outdated

 109 | +    //   with that hash will be the one to get the block request, unless no connected
 110 | +    //   nodes are known to have this block, in which case a random one will be queried.
 111 | +    void AskForBlocks(const uint256 hashEnd, const uint256 hashOriginator);
 112 | +};
 113 | +
 114 | +// A simple generic CHub Listening class which can be extended, if you wish

sipa commented at 2:51 PM on June 8, 2012:

Perhaps we could have a class CHubClient, which has a pointer to a CHub in a protected field, and public methods EmitSome, which forward to the connected CHub (or maybe the EmitSome method's implementations could even be moved to CHubCLient). class CHubListener could then extend CHubClient and reuse the phub pointer in it.

Advantage: inside CWallet, you can simply call Emit* directory without qualifying with the phub pointer, while guaranteeing that the wallet will get callbacks from the same hub as the one it submits to.

TheBlueMatt commented at 8:38 PM on June 10, 2012:

I had originally made CHubListener do that as well, but, as it turns out, we only ever call EmitTransaction from CWalletTx::AcceptWalletTransaction (which calls EmitTransaction on each supporting transaction first), and making the hub pointer public so that CWalletTx can call pwallet->phub->EmitTransaction seems kinda ugly to me. Thus a CHubClient would be unused (currently), but ACK on the idea probably for a P2P/Net class. One could move AcceptWalletTransaction code to CWallet, as its only used twice, but I find that to also be ugly... In the end, since having more than one CHub in a given process is entirely unsupported, it seems cleaner to just use the global phub, but it wouldnt be hard to pass a phub pointer to CWalletTx::EmitTransaction if we wanted to support that (though a CHubClient::phub should be private, whereas that would require it be protected, so, again, I find that ugly...)

TheBlueMatt commented at 8:15 PM on July 5, 2012: member

This needs rebasing, and Im not going to keep rebasing this stuff without any interest in eventually merging. If it ever gets interest, I may reopen.

TheBlueMatt closed this on Jul 5, 2012

suprnurd referenced this in commit 559f8421b1 on Dec 5, 2017

lateminer referenced this in commit ec0217a2ee on Jan 22, 2019

lateminer referenced this in commit 8bd8c2ca93 on May 6, 2020

DrahtBot locked this on Sep 8, 2021