Simple realtime log of p2p network activity (blocks and transactions only)
547 | + assert(!logpos); 548 | + log.push_back(le); 549 | + } 550 | + ++rows_used; 551 | + if (rows_used > max_nonweak_txouts) { 552 | + LogEntry& le = getLogEntryRow(rows_used - max_nonweak_txouts - 1);
le is the same name as the first argument here, it will bring shadow warning.
This is how it looks like here on testnet:
<img width="1167" alt="screen shot 2017-02-24 at 08 22 39" src="https://cloud.githubusercontent.com/assets/6848764/23294190/86f5186e-fa6a-11e6-83bd-dfa5811b8b68.png">
Thanks. Will review. I just played a bit with it and had massive locking issues on mainnet during catch-up of 2-3 weeks.
Running this PR (built over gitian: https://bitcoin.jonasschnelli.ch/build/54) makes Bitcoin-Qt and also the rest of my apps almost unusable. Had to force kill the process.
614 | +} 615 | + 616 | +void NetWatchLogModel::LogBlock(const CBlockIndex* pblockindex) 617 | +{ 618 | + CBlock block; 619 | + if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus())) {
IMO reading each block will cause a massive slow down during IBD / catchup.
The OS should have the data cached already?
618 | + CBlock block; 619 | + if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus())) { 620 | + // Indicate error somehow? 621 | + return; 622 | + } 623 | + assert(block.vtx.size());
What we probably should do is adding each blocks size and vtx.size() to CBlockIndex* (would require to alter the block index, migration, etc. yes).
That wouldn't change anything here...?
Concept ACK, I like having this, though we have to be sure that there is no performance impact when the monitor is not running, and as little as possible when it is.
The code is (already) disabled until the first time it is opened.
Oops, had rebased onto 0.17 instead of master. Rebased for real this time.
676 | + if (clientModel) { 677 | + disconnect(clientModel->getOptionsModel(), SIGNAL(displayUnitChanged(int)), this, SLOT(updateDisplayUnit())); 678 | + } 679 | + clientModel = model; 680 | + if (model) { 681 | + connect(model->getOptionsModel(), SIGNAL(displayUnitChanged(int)), this, SLOT(updateDisplayUnit()));
Might want to use the new connect syntax in this file?
Let me know if I got it right...
683 | + NetWatch->setClientModel(clientModel); 684 | + } 685 | + NetWatch->showNormal(); 686 | + NetWatch->show(); 687 | + NetWatch->raise(); 688 | + NetWatch->activateWindow();
73 | @@ -73,7 +74,8 @@ const std::string BitcoinGUI::DEFAULT_UIPLATFORM = 74 | BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformStyle, const NetworkStyle *networkStyle, QWidget *parent) : 75 | QMainWindow(parent), 76 | m_node(node), 77 | - platformStyle(_platformStyle) 78 | + platformStyle(_platformStyle), 79 | + netStyle(networkStyle)
Remove.
It's needed...
Sorry, forgot to delete this comment while reviewing..
0 | @@ -0,0 +1,228 @@ 1 | +// Copyright (c) 2017 The Bitcoin Core developers
nit 2018.
2018 is wrong. This code hasn't been substantially changed since 2017.
21 | +class CBlock; 22 | +class CBlockIndex; 23 | +class ClientModel; 24 | +class LogEntry; 25 | +class PlatformStyle; 26 | +class NetworkStyle;
nit, sort.
23 | +class ClientModel; 24 | +class LogEntry; 25 | +class PlatformStyle; 26 | +class NetworkStyle; 27 | + 28 | +static const int nLongestAddress = 35;
move to .cpp? nit, follow code convention.
403 | @@ -397,6 +404,7 @@ void BitcoinGUI::createMenuBar() 404 | settings->addAction(optionsAction); 405 | 406 | QMenu *help = appMenuBar->addMenu(tr("&Help")); 407 | + help->addAction(NetWatchAction);
127 | @@ -127,6 +128,7 @@ class BitcoinGUI : public QMainWindow 128 | QAction* backupWalletAction = nullptr; 129 | QAction* changePassphraseAction = nullptr; 130 | QAction* aboutQtAction = nullptr; 131 | + QAction* NetWatchAction = nullptr;
nit, m_show_netwatch_action.
71 | + ++count; 72 | + } 73 | + return NULL; 74 | +} 75 | + 76 | +}
nit, } // namespace
73 | + return NULL; 74 | +} 75 | + 76 | +} 77 | + 78 | +const QString LogEntry::LogEntryTypeAbbreviation(const LogEntryType LET)
nit, s/LET/log_entry_type.
47 | +} 48 | + 49 | +size_t CountNonDatacarrierOutputs(const CTransactionRef& tx) 50 | +{ 51 | + size_t count = 0; 52 | + for (auto& txout : tx->vout) {
const?
48 | + 49 | +size_t CountNonDatacarrierOutputs(const CTransactionRef& tx) 50 | +{ 51 | + size_t count = 0; 52 | + for (auto& txout : tx->vout) { 53 | + if (IsDatacarrier(txout)) {
nit, one line if (...) continue;
60 | + 61 | +const CTxOut* GetNonDatacarrierOutput(const CTransactionRef& tx, const size_t txout_index) 62 | +{ 63 | + size_t count = 0; 64 | + for (auto& txout : tx->vout) { 65 | + if (IsDatacarrier(txout)) {
Same as above.
63 | + size_t count = 0; 64 | + for (auto& txout : tx->vout) { 65 | + if (IsDatacarrier(txout)) { 66 | + continue; 67 | + } 68 | + if (count == txout_index) {
Same as above.
IMO this one is clearer split out.
68 | + if (count == txout_index) { 69 | + return &txout; 70 | + } 71 | + ++count; 72 | + } 73 | + return NULL;
nullptr.
0 | @@ -0,0 +1,838 @@ 1 | +// Copyright (c) 2017 The Bitcoin Core developers
nit, 2018
Concept ACK.
Code style is a bit outdated.
260 | + CTransactionWeakref * const ptx_new = get<CTransactionWeakref>(); 261 | + new (ptx_new) CTransactionWeakref(tx); 262 | + *((meta_t*)data) |= (3 << 30); 263 | +} 264 | + 265 | +class NetWatchValidationInterface final : public CValidationInterface {
Should use/extend interfaces::Node instead?
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
75 | + LET_TX, 76 | + }; 77 | + static const QString LogEntryTypeAbbreviation(LogEntryType); 78 | + 79 | + LogEntry(const LogEntry&); 80 | + LogEntry() : data(NULL) { }
Use nullptr instead of NULL. Applies throughout this PR :-)
12 | +#include <qt/clientmodel.h> 13 | +#include <qt/guiconstants.h> 14 | +#include <qt/guiutil.h> 15 | +#include <qt/optionsmodel.h> 16 | +#include <qt/platformstyle.h> 17 | +#include <qt/networkstyle.h>
Nit: Sort includes :-)
114 | + bool fCheckType; 115 | + bool fCheckId; 116 | + bool fCheckAddr; 117 | + bool fCheckValue; 118 | + 119 | + NetWatchLogSearch(const QString& query, int displayunit);
Nit: Make parameter names match between declaration and definition.
261 | +class NetWatchValidationInterface final : public CValidationInterface { 262 | +private: 263 | + NetWatchLogModel& model; 264 | + 265 | +public: 266 | + NetWatchValidationInterface(NetWatchLogModel& model_in) : model(model_in) {}
Should be explicit? :-)
338 | + CBlock block; 339 | + if (!ReadBlockFromDisk(block, &blockindex, Params().GetConsensus())) { 340 | + // Indicate error somehow? 341 | + return QVariant(); 342 | + } 343 | + assert(block.vtx.size());
Nit: !block.vtx.empty() communicates intention more clearly than block.vtx.size()?
Not sure. The intent is only to make sure vtx[0] below is an acceptable dereference.
The actual check for block data is above, with if (blockindex.nTx == 0 || !(blockindex.nStatus & BLOCK_HAVE_DATA))
I see -- makes sense :-)
541 | +void NetWatchLogModel::searchRows(const QString& query, QList<int>& results) 542 | +{ 543 | + const auto currentUnit = clientModel->getOptionsModel()->getDisplayUnit(); 544 | + NetWatchLogSearch *newsearch = new NetWatchLogSearch(query, currentUnit); 545 | + LOCK(cs); 546 | + if (currentSearch) {
This if statement is unnecessary. Deleting a null pointer is a noop.
155 | + NWLMH_ID, 156 | + NWLMH_ADDR, 157 | + NWLMH_VALUE, 158 | + }; 159 | + 160 | + NetWatchLogModel(QWidget *parent);
Should be made explicit?
273 | +void NetWatchValidationInterface::ValidationInterfaceUnregistering() 274 | +{ 275 | + model.OrphanedValidationInterface(); 276 | +} 277 | + 278 | +void NetWatchValidationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload)
Should be marked override?
qt/netwatch.cpp:273:70: error: virt-specifiers in ‘ValidationInterfaceUnregistering’ not allowed outside a class definition
Adding it to only the class def instead...
Sounds good. I commented on the wrong line :-)
51 | + offset = size_t(p); 52 | + return offset + sizeof(T); 53 | + } 54 | + 55 | + // std::align (missing in at least GCC 4.9) substitute from c-plus 56 | + static inline void *align( std::size_t alignment, std::size_t size, void *&ptr, std::size_t &space ) {
Remove extra spaces after ( and before ) :-)
572 | + // Replace a deleted row 573 | + getLogEntryRow(rows_used) = le; 574 | + --m_logskip; 575 | + } else { 576 | + // Haven't filled up yet, so just push_back 577 | + assert(!m_logpos);
I see this assertion on Windows in testing, haven't take a look at the code.
Any idea how?>
403 | @@ -397,6 +404,7 @@ void BitcoinGUI::createMenuBar() 404 | settings->addAction(optionsAction); 405 | 406 | QMenu *help = appMenuBar->addMenu(tr("&Help")); 407 | + help->addAction(m_show_netwatch_action);
Simple realtime log of p2p network activity (blocks and transactions only)
- Doesn't begin logging until opened; limited to 0x400 entries (outputs)
- Automatically scrolls if left at the bottom of the log; maintains position if left elsewhere
- Memory-efficient circular buffer; CTransaction references become weak after they're 0x200 entries back in the log
- Search function that selects all matching log entries, including ongoing
253 | + CTransactionRef * const ptx_old = get<CTransactionRef>(); 254 | + CTransactionRef tx = *ptx_old; // save a copy 255 | + ptx_old->~CTransactionRef(); 256 | + CTransactionWeakref * const ptx_new = get<CTransactionWeakref>(); 257 | + new (ptx_new) CTransactionWeakref(tx); 258 | + *((meta_t*)m_data) |= (3 << 30);
Shift overflow?
<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase
As this PR has been inactive for a long time, I'm closing this (but marking "up for grabs").