and add the walletutil.h include header for WALLET_FLAG_AVOID_REUSE that was
already missing before this change.
WALLET_FLAG_CAVEATS is only used in one RPC, so no need to encumber wallet.h and
wallet.cpp with it, along with all of the files that include wallet.h during
their compilation. Also apply clang-format per:
git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v
This new "warnings" field is a JSON array of strings intended to replace the
"warning" string field in these four RPCs, to better handle returning multiple
warning messages and for consistency with other wallet RPCs.
and clarify the intention behind the -nodebuglogfile bench.
Co-authored-by: "kouloumos <kouloumosa@gmail.com>"
Co-authored-by: "Larry Ruane <larryruane@gmail.com>"
that was resolved in PR27036 "test: Remove last uses of snprintf and simplify"
and while here, fix up 2 words in docs to make the spelling linter green again.
During connection setup for a peer, getpeerinfo returns "version": 0, "subver": ""
and the GUI Peers window displays 0 and an empty field, respectively.
Give these fields the same behavior as the other fields in the GUI Peers window:
display the fallback value in src/qt/forms/debugwindow.ui (i.e. "N/A") until a
valid result is available after the peer connection completes.
to the current p2p behavior. We only initialize the Peer::TxRelay m_relay_txs
data structure if it isn't an outbound block-relay-only connection and fRelay=true
(the peer wishes to receive tx announcements) or we're offering NODE_BLOOM to this peer.
and also fix spelling in test/lint/lint-locale-dependence.py not caught by the
spelling linter and fix up a paragraph we are touching here in test/README.md.
for verbose log messages for development or debugging only, as bitcoind may run
more slowly, that are more granular/frequent than the Debug log level, i.e. for
very high-frequency, low-level messages to be logged distinctly from
higher-level, less-frequent debug logging that could still be usable in production.
An example would be to log higher-level peer events (connection, disconnection,
misbehavior, eviction) as Debug, versus Trace for low-level, high-volume p2p
messages in the BCLog::NET category. This will enable the user to log only the
former without the latter, in order to focus on high-level peer management events.
With respect to the name, "trace" is suggested as the most granular level
in resources like the following:
- https://sematext.com/blog/logging-levels
- https://howtodoinjava.com/log4j2/logging-levels
Update the test framework and add test coverage.
- simplify the BCLog::Level enum class (and future changes to it) by
only setting the value of the first enumerator
- move the BCLog::Level:None enumerator to the end of the BCLog::Level
enum class and LogLevelToStr() member function, as the None enumerator
is only used internally, and by being the highest BCLog::Level value it
can be used to iterate over the enumerators
- replace the unused BCLog::Level:None string "none" with an empty string
as the case will never be hit
- add documentation
- make the code easier to read and understand
- improve performance by avoiding unnecessary move operations
- the cleaner, simpler, and easier to read the code is, the
better chance the compiler has at implementing it well
as the classes themselves are private, and to be consistent within all the
*Impl classes in src/node/interfaces.cpp and src/wallet/interfaces.cpp
following this order:
public:
// ... virtual methods ...
// ... nonvirtual helper methods ...
// ... data members ...
and add documentation in src/node/interfaces.cpp and src/wallet/interfaces.cpp
to help future reviewers and contributors.
and mark the inherited CBlockIndex#GetBlockHash public interface member
as deleted, to disallow calling it in the derived CDiskBlockIndex class.
Here is a failing test on master demonstrating the inconsistent behavior of the
current design: calling the same inherited public interface functions on the
same CDiskBlockIndex object should yield identical behavior.
```diff
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 6dc522b421..dac3840f32 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -240,6 +240,15 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
const CBlockIndex* tip = chainman.ActiveTip();
BOOST_CHECK_EQUAL(tip->nChainTx, au_data.nChainTx);
+ // CDiskBlockIndex "is a" CBlockIndex, as it publicly inherits from it.
+ // Test that calling the same inherited interface functions on the same
+ // object yields identical behavior.
+ CDiskBlockIndex index{tip};
+ CBlockIndex *pB = &index;
+ CDiskBlockIndex *pD = &index;
+ BOOST_CHECK_EQUAL(pB->GetBlockHash(), pD->GetBlockHash());
+ BOOST_CHECK_EQUAL(pB->ToString(), pD->ToString());
+
```
The GetBlockHash() test assertion only passes on master because the different
methods invoked by the current design happen to return the same result. If one
of the two is changed, it fails like the ToString() assertion does.
Redefining inherited non-virtual functions is well-documented as incorrect
design to avoid inconsistent behavior (see Scott Meyers, "Effective C++", Item
36). Class usage is confusing when the behavior depends on the pointer
definition instead of the object definition (static binding happening where
dynamic binding was expected). This can lead to unsuspected or hard-to-track
bugs.
Outside of critical hot spots, correctness usually comes before optimisation,
but the current design dates back to main.cpp and it may possibly have been
chosen to avoid the overhead of dynamic dispatch. This solution does the same:
the class sizes are unchanged and no vptr or vtbl is added.
There are better designs for doing this that use composition instead of
inheritance or that separate the public interface from the private
implementations. One example of the latter would be a non-virtual public
interface that calls private virtual implementation methods, i.e. the Template
pattern via the Non-Virtual Interface (NVI) idiom.
and remove a now-redundant assert preceding a GetBlockHash() caller.
This protects against UB here, and in case of failure (which would
indicate a consensus bug), the debug log will print
bitcoind: chain.h:265: uint256 CBlockIndex::GetBlockHash() const: Assertion `phashBlock != nullptr' failed.
Aborted
instead of
Segmentation fault
Here we update only the log messages that manually print a category.
In upcoming commits, LogPrintCategory will likely be used in many
other cases, such as to replace `LogPrintf` where it makes sense.
that was added in 2015 by commit b8c06ef40 in PR 7003, as that potential issue
would now be caught by the test/lint/lint-format-strings.py script run by the CI
and inverse its logic.
The naming is out of date and incorrect, as lack of request of tx relay does not
imply block relay, and a preference for tx relay doesn't imply that block relay
isn't happening.
"error: JSON value is not a boolean as expected"
due to fRelayTxes/m_relay_txs being moved in PR 21160 from CNodeStats to
CNodeStateStats, which made getpeerinfo#relaytxes an optional field that
can return UniValue IsNull().
Fixes negative time duration values in the "send", "recv",
and "age" columns (potentially the "txn" and "blk" columns also)
for the first run of -rpcwait -netinfo after bitcoind startup.
To reproduce, start bitcoind on mainnet and run
`bitcoin-cli -rpcwait -netinfo <n>` where n is 1 or larger.
The negative times will be larger/more apparent with a slower
CPU speed or e.g. higher checkblocks/checklevel config option
settings.
where all the other logging actions in src/net.{h,cpp} are located.
StartExtraBlockRelayPeers() does not appear to be a hotspot that needs to be
inlined for performance, as it is called from CheckForStaleTipAndEvictPeers(),
called in turn from StartScheduledTasks() with a scheduleEvery delta of 45
seconds, called at the end of AppInitMain() on bitcoind startup.
This allows dropping `#include <logging.h>` from net.h, which can improve
compile time/speed. Currently, none of the other includes in net.h use
logging.h, except src/sync.h if DEBUG_LOCKCONTENTION is defined.
Jon Atack