Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Provide fee data for all txs in RPC getblocktemplate response #2115

Merged
merged 3 commits into from

6 participants

@forrestv

Having the fee data for every transaction returned by "getblocktemplate" was broken by the ultraprune commit (450cbb0). It made it so transactions that depend on other transactions in the block-to-be don't have fee data.

@BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/d42d932bed6b564cbfc00216d1c04cd5248ddeef for binaries and test log.

src/rpcmining.cpp
@@ -307,14 +307,15 @@ Value getblocktemplate(const Array& params, bool fHelp)
entry.push_back(Pair("depends", deps));
int64_t nSigOps = tx.GetLegacySigOpCount();
- if (tx.HaveInputs(view))
- {
- entry.push_back(Pair("fee", (int64_t)(tx.GetValueIn(view) - tx.GetValueOut())));
- nSigOps += tx.GetP2SHSigOpCount(view);
- }
+ assert(tx.HaveInputs(view));
@luke-jr
luke-jr added a note

I dislike the possibility of bitcoind just aborting. Is there some reason not to make this an if() block as a failsafe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sipa
Owner

I think it would be better to have CreateNewBlock construct some meta-data object with fees in it, which can be used by getblocktemplate.

EDIT: this definitely needs fixing of course, but it seems silly to have GBT redo the fee calculation in an incomplete way, when CreateNewBlock perfectly knows all fees.

@luke-jr

No reason it can't be optimized after merging this fix, IMO.

@forrestv

/me is working on having CreateNewBlock return fee data in addition

@BitcoinPullTester

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/3a291f97f0dc1c98f980a3f5679000339416961f for binaries and test log.

This could happen for one of several reasons:
1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
2. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
3. The test suite fails on either Linux i386 or Win32
4. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

@forrestv

Okay, CreateNewBlock now returns a CBlockTemplate struct that contains the block and fee and sigop data.

@BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/0f927ceb5b90ec02be36ddb20b2f4cff82589265 for binaries and test log.

@sipa
Owner

Code looks good to me. I'll do some testing soon.

@sipa
Owner

ACK

@gmaxwell
Owner

Been running this for almost two weeks on a node. Did basic sanity checks. ACK.

@jgarzik

No objection (see 'ACK' in overall commit), but this construction "&tx - pblock->vtx.data()" seems quite unusual. Perhaps separate it into its own code line, perhaps with a comment.

@jgarzik
Owner

ACK

@BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f3d872d1eabeb5c999162f709626ee20c8789c42 for binaries and test log.

@sipa sipa merged commit 45a1ec5 into bitcoin:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 19, 2012
  1. @forrestv

    changed CreateNewBlock to return a CBlockTemplate object, which inclu…

    forrestv authored
    …des per-tx fee and sigop count data
  2. @forrestv

    use fee/sigop data in BlockTemplate struct instead of (not always cor…

    forrestv authored
    …rectly) calculating it ourselves
Commits on Jan 4, 2013
  1. @forrestv
This page is out of date. Refresh to see the latest.
Showing with 70 additions and 55 deletions.
  1. +18 −10 src/main.cpp
  2. +10 −1 src/main.h
  3. +19 −22 src/rpcmining.cpp
  4. +23 −22 src/test/miner_tests.cpp
View
28 src/main.cpp
@@ -3732,12 +3732,13 @@ class TxPriorityCompare
}
};
-CBlock* CreateNewBlock(CReserveKey& reservekey)
+CBlockTemplate* CreateNewBlock(CReserveKey& reservekey)
{
// Create new block
- auto_ptr<CBlock> pblock(new CBlock());
- if (!pblock.get())
+ auto_ptr<CBlockTemplate> pblocktemplate(new CBlockTemplate());
+ if(!pblocktemplate.get())
return NULL;
+ CBlock *pblock = &pblocktemplate->block; // pointer for convenience
// Create coinbase tx
CTransaction txNew;
@@ -3748,6 +3749,8 @@ CBlock* CreateNewBlock(CReserveKey& reservekey)
// Add our coinbase tx as first transaction
pblock->vtx.push_back(txNew);
+ pblocktemplate->vTxFees.push_back(-1); // updated at end
+ pblocktemplate->vTxSigOps.push_back(-1); // updated at end
// Largest block you're willing to create:
unsigned int nBlockMaxSize = GetArg("-blockmaxsize", MAX_BLOCK_SIZE_GEN/2);
@@ -3925,6 +3928,8 @@ CBlock* CreateNewBlock(CReserveKey& reservekey)
// Added
pblock->vtx.push_back(tx);
+ pblocktemplate->vTxFees.push_back(nTxFees);
+ pblocktemplate->vTxSigOps.push_back(nTxSigOps);
nBlockSize += nTxSize;
++nBlockTx;
nBlockSigOps += nTxSigOps;
@@ -3959,13 +3964,15 @@ CBlock* CreateNewBlock(CReserveKey& reservekey)
printf("CreateNewBlock(): total size %"PRI64u"\n", nBlockSize);
pblock->vtx[0].vout[0].nValue = GetBlockValue(pindexPrev->nHeight+1, nFees);
+ pblocktemplate->vTxFees[0] = -nFees;
// Fill in header
pblock->hashPrevBlock = pindexPrev->GetBlockHash();
pblock->UpdateTime(pindexPrev);
- pblock->nBits = GetNextWorkRequired(pindexPrev, pblock.get());
+ pblock->nBits = GetNextWorkRequired(pindexPrev, pblock);
pblock->nNonce = 0;
pblock->vtx[0].vin[0].scriptSig = CScript() << OP_0 << OP_0;
+ pblocktemplate->vTxSigOps[0] = pblock->vtx[0].GetLegacySigOpCount();
CBlockIndex indexDummy(*pblock);
indexDummy.pprev = pindexPrev;
@@ -3975,7 +3982,7 @@ CBlock* CreateNewBlock(CReserveKey& reservekey)
throw std::runtime_error("CreateNewBlock() : ConnectBlock failed");
}
- return pblock.release();
+ return pblocktemplate.release();
}
@@ -4118,10 +4125,11 @@ void static BitcoinMiner(CWallet *pwallet)
unsigned int nTransactionsUpdatedLast = nTransactionsUpdated;
CBlockIndex* pindexPrev = pindexBest;
- auto_ptr<CBlock> pblock(CreateNewBlock(reservekey));
- if (!pblock.get())
+ auto_ptr<CBlockTemplate> pblocktemplate(CreateNewBlock(reservekey));
+ if (!pblocktemplate.get())
return;
- IncrementExtraNonce(pblock.get(), pindexPrev, nExtraNonce);
+ CBlock *pblock = &pblocktemplate->block;
+ IncrementExtraNonce(pblock, pindexPrev, nExtraNonce);
printf("Running BitcoinMiner with %"PRIszu" transactions in block (%u bytes)\n", pblock->vtx.size(),
::GetSerializeSize(*pblock, SER_NETWORK, PROTOCOL_VERSION));
@@ -4134,7 +4142,7 @@ void static BitcoinMiner(CWallet *pwallet)
char pdatabuf[128+16]; char* pdata = alignup<16>(pdatabuf);
char phash1buf[64+16]; char* phash1 = alignup<16>(phash1buf);
- FormatHashBuffers(pblock.get(), pmidstate, pdata, phash1);
+ FormatHashBuffers(pblock, pmidstate, pdata, phash1);
unsigned int& nBlockTime = *(unsigned int*)(pdata + 64 + 4);
unsigned int& nBlockBits = *(unsigned int*)(pdata + 64 + 8);
@@ -4170,7 +4178,7 @@ void static BitcoinMiner(CWallet *pwallet)
assert(hash == pblock->GetHash());
SetThreadPriority(THREAD_PRIORITY_NORMAL);
- CheckWork(pblock.get(), *pwalletMain, reservekey);
+ CheckWork(pblock, *pwalletMain, reservekey);
SetThreadPriority(THREAD_PRIORITY_LOWEST);
break;
}
View
11 src/main.h
@@ -108,6 +108,8 @@ class CTxUndo;
class CCoinsView;
class CCoinsViewCache;
+struct CBlockTemplate;
+
/** Register a wallet to receive updates from core */
void RegisterWallet(CWallet* pwalletIn);
/** Unregister a wallet from core */
@@ -139,7 +141,7 @@ void ThreadImport(void *parg);
/** Run the miner threads */
void GenerateBitcoins(bool fGenerate, CWallet* pwallet);
/** Generate a new block, without valid proof-of-work */
-CBlock* CreateNewBlock(CReserveKey& reservekey);
+CBlockTemplate* CreateNewBlock(CReserveKey& reservekey);
/** Modify the extranonce in a block */
void IncrementExtraNonce(CBlock* pblock, CBlockIndex* pindexPrev, unsigned int& nExtraNonce);
/** Do mining precalculation */
@@ -1975,4 +1977,11 @@ extern CCoinsViewCache *pcoinsTip;
/** Global variable that points to the active block tree (protected by cs_main) */
extern CBlockTreeDB *pblocktree;
+struct CBlockTemplate
+{
+ CBlock block;
+ std::vector<int64_t> vTxFees;
+ std::vector<int64_t> vTxSigOps;
+};
+
#endif
View
41 src/rpcmining.cpp
@@ -103,7 +103,7 @@ Value getwork(const Array& params, bool fHelp)
typedef map<uint256, pair<CBlock*, CScript> > mapNewBlock_t;
static mapNewBlock_t mapNewBlock; // FIXME: thread safety
- static vector<CBlock*> vNewBlock;
+ static vector<CBlockTemplate*> vNewBlockTemplate;
static CReserveKey reservekey(pwalletMain);
if (params.size() == 0)
@@ -112,7 +112,7 @@ Value getwork(const Array& params, bool fHelp)
static unsigned int nTransactionsUpdatedLast;
static CBlockIndex* pindexPrev;
static int64 nStart;
- static CBlock* pblock;
+ static CBlockTemplate* pblocktemplate;
if (pindexPrev != pindexBest ||
(nTransactionsUpdated != nTransactionsUpdatedLast && GetTime() - nStart > 60))
{
@@ -120,9 +120,9 @@ Value getwork(const Array& params, bool fHelp)
{
// Deallocate old blocks since they're obsolete now
mapNewBlock.clear();
- BOOST_FOREACH(CBlock* pblock, vNewBlock)
- delete pblock;
- vNewBlock.clear();
+ BOOST_FOREACH(CBlockTemplate* pblocktemplate, vNewBlockTemplate)
+ delete pblocktemplate;
+ vNewBlockTemplate.clear();
}
// Clear pindexPrev so future getworks make a new block, despite any failures from here on
@@ -134,14 +134,15 @@ Value getwork(const Array& params, bool fHelp)
nStart = GetTime();
// Create new block
- pblock = CreateNewBlock(reservekey);
- if (!pblock)
+ pblocktemplate = CreateNewBlock(reservekey);
+ if (!pblocktemplate)
throw JSONRPCError(RPC_OUT_OF_MEMORY, "Out of memory");
- vNewBlock.push_back(pblock);
+ vNewBlockTemplate.push_back(pblocktemplate);
// Need to update only after we know CreateNewBlock succeeded
pindexPrev = pindexPrevNew;
}
+ CBlock* pblock = &pblocktemplate->block; // pointer for convenience
// Update nTime
pblock->UpdateTime(pindexPrev);
@@ -248,7 +249,7 @@ Value getblocktemplate(const Array& params, bool fHelp)
static unsigned int nTransactionsUpdatedLast;
static CBlockIndex* pindexPrev;
static int64 nStart;
- static CBlock* pblock;
+ static CBlockTemplate* pblocktemplate;
if (pindexPrev != pindexBest ||
(nTransactionsUpdated != nTransactionsUpdatedLast && GetTime() - nStart > 5))
{
@@ -261,18 +262,19 @@ Value getblocktemplate(const Array& params, bool fHelp)
nStart = GetTime();
// Create new block
- if(pblock)
+ if(pblocktemplate)
{
- delete pblock;
- pblock = NULL;
+ delete pblocktemplate;
+ pblocktemplate = NULL;
}
- pblock = CreateNewBlock(reservekey);
- if (!pblock)
+ pblocktemplate = CreateNewBlock(reservekey);
+ if (!pblocktemplate)
throw JSONRPCError(RPC_OUT_OF_MEMORY, "Out of memory");
// Need to update only after we know CreateNewBlock succeeded
pindexPrev = pindexPrevNew;
}
+ CBlock* pblock = &pblocktemplate->block; // pointer for convenience
// Update nTime
pblock->UpdateTime(pindexPrev);
@@ -281,7 +283,6 @@ Value getblocktemplate(const Array& params, bool fHelp)
Array transactions;
map<uint256, int64_t> setTxIndex;
int i = 0;
- CCoinsViewCache &view = *pcoinsTip;
BOOST_FOREACH (CTransaction& tx, pblock->vtx)
{
uint256 txHash = tx.GetHash();
@@ -306,13 +307,9 @@ Value getblocktemplate(const Array& params, bool fHelp)
}
entry.push_back(Pair("depends", deps));
- int64_t nSigOps = tx.GetLegacySigOpCount();
- if (tx.HaveInputs(view))
- {
- entry.push_back(Pair("fee", (int64_t)(tx.GetValueIn(view) - tx.GetValueOut())));
- nSigOps += tx.GetP2SHSigOpCount(view);
- }
- entry.push_back(Pair("sigops", nSigOps));
+ int index_in_template = &tx - pblock->vtx.data();
+ entry.push_back(Pair("fee", pblocktemplate->vTxFees[index_in_template]));
+ entry.push_back(Pair("sigops", pblocktemplate->vTxSigOps[index_in_template]));
transactions.push_back(entry);
}
View
45 src/test/miner_tests.cpp
@@ -49,19 +49,20 @@ struct {
BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
{
CReserveKey reservekey(pwalletMain);
- CBlock *pblock;
+ CBlockTemplate *pblocktemplate;
CTransaction tx;
CScript script;
uint256 hash;
// Simple block creation, nothing special yet:
- BOOST_CHECK(pblock = CreateNewBlock(reservekey));
+ BOOST_CHECK(pblocktemplate = CreateNewBlock(reservekey));
// We can't make transactions until we have inputs
// Therefore, load 100 blocks :)
std::vector<CTransaction*>txFirst;
for (unsigned int i = 0; i < sizeof(blockinfo)/sizeof(*blockinfo); ++i)
{
+ CBlock *pblock = &pblocktemplate->block; // pointer for convenience
pblock->nVersion = 1;
pblock->nTime = pindexBest->GetMedianTimePast()+1;
pblock->vtx[0].vin[0].scriptSig = CScript();
@@ -75,10 +76,10 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
assert(ProcessBlock(NULL, pblock));
pblock->hashPrevBlock = pblock->GetHash();
}
- delete pblock;
+ delete pblocktemplate;
// Just to make sure we can still make simple blocks
- BOOST_CHECK(pblock = CreateNewBlock(reservekey));
+ BOOST_CHECK(pblocktemplate = CreateNewBlock(reservekey));
// block sigops > limit: 1000 CHECKMULTISIG + 1
tx.vin.resize(1);
@@ -95,8 +96,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
mempool.addUnchecked(hash, tx);
tx.vin[0].prevout.hash = hash;
}
- BOOST_CHECK(pblock = CreateNewBlock(reservekey));
- delete pblock;
+ BOOST_CHECK(pblocktemplate = CreateNewBlock(reservekey));
+ delete pblocktemplate;
mempool.clear();
// block size > limit
@@ -115,15 +116,15 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
mempool.addUnchecked(hash, tx);
tx.vin[0].prevout.hash = hash;
}
- BOOST_CHECK(pblock = CreateNewBlock(reservekey));
- delete pblock;
+ BOOST_CHECK(pblocktemplate = CreateNewBlock(reservekey));
+ delete pblocktemplate;
mempool.clear();
// orphan in mempool
hash = tx.GetHash();
mempool.addUnchecked(hash, tx);
- BOOST_CHECK(pblock = CreateNewBlock(reservekey));
- delete pblock;
+ BOOST_CHECK(pblocktemplate = CreateNewBlock(reservekey));
+ delete pblocktemplate;
mempool.clear();
// child with higher priority than parent
@@ -140,8 +141,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
tx.vout[0].nValue = 5900000000LL;
hash = tx.GetHash();
mempool.addUnchecked(hash, tx);
- BOOST_CHECK(pblock = CreateNewBlock(reservekey));
- delete pblock;
+ BOOST_CHECK(pblocktemplate = CreateNewBlock(reservekey));
+ delete pblocktemplate;
mempool.clear();
// coinbase in mempool
@@ -151,8 +152,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
tx.vout[0].nValue = 0;
hash = tx.GetHash();
mempool.addUnchecked(hash, tx);
- BOOST_CHECK(pblock = CreateNewBlock(reservekey));
- delete pblock;
+ BOOST_CHECK(pblocktemplate = CreateNewBlock(reservekey));
+ delete pblocktemplate;
mempool.clear();
// invalid (pre-p2sh) txn in mempool
@@ -169,8 +170,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
tx.vout[0].nValue -= 1000000;
hash = tx.GetHash();
mempool.addUnchecked(hash,tx);
- BOOST_CHECK(pblock = CreateNewBlock(reservekey));
- delete pblock;
+ BOOST_CHECK(pblocktemplate = CreateNewBlock(reservekey));
+ delete pblocktemplate;
mempool.clear();
// double spend txn pair in mempool
@@ -183,18 +184,18 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
tx.vout[0].scriptPubKey = CScript() << OP_2;
hash = tx.GetHash();
mempool.addUnchecked(hash, tx);
- BOOST_CHECK(pblock = CreateNewBlock(reservekey));
- delete pblock;
+ BOOST_CHECK(pblocktemplate = CreateNewBlock(reservekey));
+ delete pblocktemplate;
mempool.clear();
// subsidy changing
int nHeight = pindexBest->nHeight;
pindexBest->nHeight = 209999;
- BOOST_CHECK(pblock = CreateNewBlock(reservekey));
- delete pblock;
+ BOOST_CHECK(pblocktemplate = CreateNewBlock(reservekey));
+ delete pblocktemplate;
pindexBest->nHeight = 210000;
- BOOST_CHECK(pblock = CreateNewBlock(reservekey));
- delete pblock;
+ BOOST_CHECK(pblocktemplate = CreateNewBlock(reservekey));
+ delete pblocktemplate;
pindexBest->nHeight = nHeight;
}
Something went wrong with that request. Please try again.