fa2c521115 Deduplicate some block-to-JSON code. (Daniel Kraft)
Pull request description:
Some of the logic converting blocks and block headers to JSON for the blockchain RPC methods (`getblock`, `getlockheader`) was duplicated. Instead of that, the `blockToJSON` RPC method now calls `blockheaderToJSON` first, and then fills in the missing, block-specific bits explicitly.
ACKs for top commit:
laanwj:
Code review ACK fa2c521115
Tree-SHA512: 1b9b269e251d9c8c1056f253cfc2a795170d609f4c26ecc85b1ff9cdd567095a673dd89564e0d587b32dfc152ea01b2682169b018f2c9c3004c511a9998d772e
059e8ccc1e Change BOOST_CHECK to BOOST_CHECK_EQUAL to see mismatched values when a check fails. (Kiminuo)
Pull request description:
This is useful to see mismatched values when a check fails as specified in the [Boost documentation](https://www.boost.org/doc/libs/1_71_0/libs/test/doc/html/boost_test/utf_reference/testing_tool_ref/assertion_boost_level.html).
This PR would make #20744 PR's diff smaller by a bit.
ACKs for top commit:
MarcoFalke:
review ACK 059e8ccc1e
theStack:
Code Review ACK 059e8ccc1e
Tree-SHA512: 82359ef38e0d1926f12a34aeff6fde6d1d307c703a080547749b908f873c2a2f894f6f094c33470b32987c229e3a1f17f7d1e877663c53293c023bde0e7272c1
Some of the logic converting blocks and block headers to JSON for
the blockchain RPC methods (getblock, getlockheader) was duplicated.
Instead of that, the blockToJSON RPC method now calls blockheaderToJSON
first, and then fills in the missing, block-specific bits explicitly.
1bca2aa694 Introduce GetUniquePath(base) helper method to replace boost::filesystem::unique_path() which is not available in std::filesystem. (Kiminuo)
Pull request description:
This PR makes it easier in #20744 to remove our dependency on the `boost::filesystem::unique_path()` function which does not have a direct equivalent in C++17.
This PR attempts to re-implement `boost::filesystem::unique_path()` as `GetUniquePath(path)` but the implementations are not meant to be the same.
Note:
* Boost 1.75.0 implementation of `unique_path`: 9cab675b71/src/unique_path.cpp (L235)
* In the previous implementation, I attempted to add:
```cpp
fs::path GetUniquePath(const fs::path& base)
{
FastRandomContext rnd;
fs::path tmpFile = base / HexStr(rnd.randbytes(8));
return tmpFile;
}
```
to `fs.cpp` but this leads to a circular dependency: "fs -> random -> logging -> fs". That is why the modified implementation adds a new file.
ACKs for top commit:
laanwj:
Code review ACK 1bca2aa694
ryanofsky:
Code review ACK 1bca2aa694. It's a simple change and extra test coverage is nice
Tree-SHA512: f324bdf0e254160c616b5033c3ece33d87db23eb0135acee99346ade7b5cf0d30f3ceefe359a25a8e9b53ba8e4419f459c2bdd369e10fc0152ce95031d1f221c
4676a4fb5b [addrman] Don't repeat "Bucketing method was updated" log multiple times (John Newbery)
436292367c [addrman] Improve serialization comments (John Newbery)
ac3547eddd [addrman] Improve variable naming/code style of touched code. (John Newbery)
a5c9b04959 [addrman] Don't rebucket new table entries unnecessarily (John Newbery)
8062d928ce [addrman] Rename asmap version to asmap checksum (John Newbery)
009b8e0fdf [addrman] Improve variable naming/code style of touched code. (John Newbery)
b4c5fda417 [addrman] Fix new table bucketing during unserialization (John Newbery)
Pull request description:
This fixes three issues in addrman unserialization.
1. An addrman entry can appear in up to 8 new table buckets. We store this entry->bucket indexing during shutdown so that on restart we can restore the entries to their correct buckets. Commit ec45646de9 broke the deserialization code so that each entry could only be put in up to one new bucket.
2. Unserialization may result in an entry appearing in a 9th bucket. If the entry already appears in 8 buckets don't try to place it in another bucket.
3. We unnecessarily rebucket when reading a peers.dat with file version 1. Don't do that.
ACKs for top commit:
vasild:
ACK 4676a4fb5b
glozow:
re-ACK 4676a4fb5b, changes were a rename, comments, and removing repeat-logging.
naumenkogs:
ACK 4676a4f
laanwj:
Code review ACK 4676a4fb5b
dhruv:
ACK 4676a4fb5b
ryanofsky:
Code review ACK 4676a4fb5b. I'm not previously familiar with this code but all the changes here do make sense and seem like improvements. Left some notes and comments, but they aren't important so feel to ignore.
Tree-SHA512: b228984f6dec5910be23c3740ae20258da33bcf66ceb7edb10e5a53163450f743bab349e47f09808b7e8d40f27143119ec3e0981d7e678aa494d8559a1c99c23
723eb4326b test: Fix Windows cross build (Hennadii Stepanov)
Pull request description:
On master (e51f6c4dee, after #20936 merge), Windows cross compiling fails:
```
$ make > /dev/null
In file included from ./policy/fees.h:12,
from policy/fees.cpp:6:
policy/fees.cpp: In member function ‘unsigned int CBlockPolicyEstimator::HighestTargetTracked(FeeEstimateHorizon) const’:
./sync.h:232:104: warning: control reaches end of non-void function [-Wreturn-type]
232 | #define LOCK(cs) DebugLock<decltype(cs)> PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__)
| ^
policy/fees.cpp:680:5: note: in expansion of macro ‘LOCK’
680 | LOCK(m_cs_fee_estimator);
| ^~~~
test/fuzz/netaddress.cpp:12:10: fatal error: netinet/in.h: No such file or directory
12 | #include <netinet/in.h>
| ^~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [Makefile:13039: test/fuzz/fuzz-netaddress.o] Error 1
make[2]: *** Waiting for unfinished jobs....
libtool: warning: undefined symbols not allowed in x86_64-w64-mingw32 shared libraries; building static only
test/fuzz/string.cpp: In function ‘void string_fuzz_target(FuzzBufferType)’:
test/fuzz/string.cpp:81:11: error: ‘ShellEscape’ was not declared in this scope
81 | (void)ShellEscape(random_string_1);
| ^~~~~~~~~~~
make[2]: *** [Makefile:13543: test/fuzz/fuzz-string.o] Error 1
make[1]: *** [Makefile:15078: all-recursive] Error 1
make: *** [Makefile:812: all-recursive] Error 1
```
This PR fixes both of errors.
ACKs for top commit:
MarcoFalke:
cr ACK 723eb4326b
Tree-SHA512: 5d2fba5ca806e64bf92011786d1f868c6624f786bfa753a10316feab7a802a28ec27a4bd25fc26dc289a399895a521c3878ffa1efeff0e540c7245cdb8e4942c
fa362064e3 rpc: Return total fee in mempool (MarcoFalke)
Pull request description:
This avoids having to loop over the whole mempool to query each entry's fee
ACKs for top commit:
achow101:
ACK fa362064e3
glozow:
ACK fa362064e3🧸
jnewbery:
ACK fa362064e3
Tree-SHA512: e2fa1664df39c9e187f9229fc35764ccf436f6f75889c5a206d34fff473fc21efbf2bb143f4ca7895c27659218c22884d0ec4195e7a536a5a96973fc9dd82d08
This fixes issue #19388. The changes are as follows:
- Add a new flag to configure, --enable-fuzz-binary, which allows building test/fuzz/fuzz regardless of whether we are building to do actual fuzzing
- Set -DPROVIDE_MAIN_FUNCTION whenever --enable-fuzz is no
- Add the following libraries to FUZZ_SUITE_LD_COMMON:
- LIBBITCOIN_WALLET
- SQLLITE_LIBS
- BDB_LIBS
- if necessary, some or all of:
- NATPMP_LIBS
- MINIUPNPC_LIBS
- LIBBITCOIN_ZMQ / ZMQ_LIBS
506e6585a5 gui: display plain "Inbound" in peer details (Jon Atack)
Pull request description:
Alternative version to #201.
ACKs for top commit:
MarcoFalke:
ACK 506e6585a5
jonasschnelli:
utACK 506e6585a5
Tree-SHA512: 88d141b14684c1dcdff47f7ba241e5a7c42c14da3d9aaa89f1649235a64fd26bc5a6055707dc07992cd9d8c05d143754f6dd51ccee69fd4309336dd07c52e61c
faf7d7418c fuzz: Avoid extraneous copy of input data, using Span<> (MarcoFalke)
Pull request description:
Seeing speedup here in the fuzz framework part (non-fuzz-target part). Speedup is only visible for input data larger than 100kB.
ACKs for top commit:
practicalswift:
cr ACK faf7d7418c: patch looks correct :)
laanwj:
Code review ACK faf7d7418c
Tree-SHA512: 41af7118846e0dfee237a6d5269a6c7cfbc775d7bd1cc2a85814cb60f6c2b37fe7fd35f1a788d4f08e6e0202c48b71054b67d2931160c445c79fc59e5347dadf
747cb5b994 netinfo: display only outbound block relay counts (Jon Atack)
76d198a5c1 netinfo: add i2p network (Jon Atack)
9d6aeca2c5 netinfo: add bip152 high-bandwidth to/from fields (Jon Atack)
5de7a6cf63 netinfo: display manual peers count (Jon Atack)
d3cca3be63 netinfo: update to use peer connection types (Jon Atack)
62bf5b7850 netinfo: add ConnectionTypeForNetinfo member helper function (Jon Atack)
Pull request description:
Merry Bitcoin Christmas! Ho ho ho 🎄✨
This PR updates `-netinfo` to:
- use the getpeerinfo `connection_type` field (and no longer use getpeerinfo `relaytxes` for block-relay detection)
- display manual peers count, if any, in the outbound row
- display the block relay counts in the outbound row only
- display high-bandwidth BIP152 compact block relay peers (`hb` column, to `.` and from `*`)
- add support for displaying I2P network peers, if any are present
Testing and review welcome! How to test:
- to run the full live dashboard (on Linux): `$ watch --interval 1 --no-title ./src/bitcoin-cli -netinfo 4`
- to run the full dashboard: ``$ ./src/bitcoin-cli -netinfo 4``
- to see the help: `$ ./src/bitcoin-cli -netinfo help`
- to see the help summary: `$ ./src/bitcoin-cli -help | grep -A4 netinfo`
ACKs for top commit:
laanwj:
re-ACK 747cb5b994
michaelfolkson:
ACK 747cb5b994
jonasschnelli:
Tested ACK 747cb5b994 - works nicely. Great that this PR only changes bitcoin-cli.
Tree-SHA512: 48fe23dddf3005a039190fcbc84167cd25b0a63489617fe14ea5db9a641a829b46b6e8dc7924aab6577d82a13909d157e82f715bd2ed3a8a15071957c35c19f3
e1e6714832 doc: refer to BIPs 339/155 in feature negotiation (Jon Atack)
Pull request description:
of `wtxidrelay` and `addrv2`/`sendaddrv2`, and add `fSuccessfullyConnected` doxygen documentation to clarify that it is set to true on VERACK.
ACKs for top commit:
laanwj:
re-ACK e1e6714832
Tree-SHA512: 3e6af5b246e4ee1ec68ee34db525746717871bc986ad4840f5a8edce55740768389f6fd0ec69046eda2fb4c69939440a96571f79d36e6cbff4fd3b7f2ebc74c0
eecb7ab105 [doc] clarify -peertimeout and -timeout descriptions (gzhao408)
Pull request description:
The debug-only option `-peertimeout` is used to delay `InactivityCheck()`, whereas the `-timeout` option specifies socket timeouts (`nConnectTimeout`). The current descriptions are a bit misleading and hard to tell apart. I think it would save dev/review time to update them 🤷
ACKs for top commit:
MarcoFalke:
ACK eecb7ab105 nice doc fixup
jnewbery:
ACK eecb7ab105
Tree-SHA512: 71d2e6c31664b9f7f0b053ecf3be21c6c55472553fa7478d8526ba3be8d54979bceafca63d87b8b2488c11f409c332ac795da613ff8101546b18d9cd8bcceb50
20677ffa22 validation: Guard all chainstates with cs_main (Carl Dong)
Pull request description:
```
This avoids a potential race-condition where a thread is reading the
ChainstateManager::m_active_chainstate pointer while another one is
writing to it. There is no portable guarantee that reading/writing the
pointer is thread-safe.
This is also done in way that mimics ::ChainstateActive(), so the
transition from that function to this method is easy.
More discussion:
1. https://github.com/bitcoin/bitcoin/pull/20749#discussion_r559544027
2. https://github.com/bitcoin/bitcoin/pull/19806#discussion_r561023961
3. https://github.com/bitcoin/bitcoin/pull/19806#issuecomment-768946522
4. https://github.com/bitcoin/bitcoin/pull/19806#issuecomment-768955695
```
Basically this PR removes the loaded-but-unfired footgun, which:
- Is multiplied (but still unshot) in the chainman deglobalization PRs (#20158)
- Is shot in the test framework in the au.activate PR (#19806)
ACKs for top commit:
jnewbery:
code review ACK 20677ffa22. I've verified by eye that neither of these members are accessed without cs_main.
ryanofsky:
Code review ACK 20677ffa22. It is safer to have these new `GUARDED_BY` annotations and locks than not to have them, but in the longer run I think every `LOCK(cs_main)` added here and added earlier in f92dc6557a from #20749 should be removed and replaced with `EXCLUSIVE_LOCKS_REQUIRED(cs_main)` on the accessor methods instead. `cs_main` is a high level lock that should be explicitly acquired at a high level to prevent the chain state from changing. It shouldn't be acquired recursively in low-level methods just to read pointer values atomically.
Tree-SHA512: 68a3a46d79a407b774eab77e1d682a97e95f1672db0a5fcb877572e188bec09f3a7b47c5d0cc1f2769ea276896dcbe97cb35c861acf7d8e3e513e955dc773f89
fa61b9d1a6 util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke)
7777105a24 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke)
fa06bce4ac test: Add tests (MarcoFalke)
fac05ccdad wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke)
Pull request description:
This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937
Fixes: #20902
ACKs for top commit:
ajtowns:
ACK fa61b9d1a6
fjahr:
Code review ACK fa61b9d1a6
Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b
the i2p peer counts column is displayed iff the node is connected
to at least one i2p peer, so this doesn't add clutter for users
who are not running an i2p service
572fd0f738 doc: More precise -debug and -debugexclude doc (wodry)
Pull request description:
I wondered how one could enable debug logging with `-debug=<category>` for multiple categories. Found out solution is to specify that option multiple times for each wanted category.
This PR documents this behavior and uses the same wording for the same behavior of `-debugexclude=<category>` to make that also clear and stringent.
ACKs for top commit:
laanwj:
ACK 572fd0f738
MarcoFalke:
ACK 572fd0f738
theStack:
ACK 572fd0f738
Tree-SHA512: 8d93db37602fd5ff4247e7c11478e55b99c0e3d47eaa2bb901937805d8f2a466b3a198b713b760981c5411576b74c52e2909c46c6d3f2e0e04215fd521b65cf7
bff7c66e67 Add documentation to contrib folder (Troy Giorshev)
381f77be85 Add Message Capture Test (Troy Giorshev)
e4f378a505 Add capture parser (Troy Giorshev)
4d1a582549 Call CaptureMessage at appropriate locations (Troy Giorshev)
f2a77ff97b Add CaptureMessage (Troy Giorshev)
dbf779d5de Clean PushMessage and ProcessMessages (Troy Giorshev)
Pull request description:
This PR introduces per-peer message capture into Bitcoin Core. 📓
## Purpose
The purpose and scope of this feature is intentionally limited. It answers a question anyone new to Bitcoin's P2P protocol has had: "Can I see what messages my node is sending and receiving?".
## Functionality
When a new debug-only command line argument `capturemessages` is set, any message that the node receives or sends is captured. The capture occurs in the MessageHandler thread. When receiving a message, it is captured as soon as the MessageHandler thread takes the message off of the vProcessMsg queue. When sending, the message is captured just before the message is pushed onto the vSendMsg queue.
The message capture is as minimal as possible to reduce the performance impact on the node. Messages are captured to a new `message_capture` folder in the datadir. Each node has their own subfolder named with their IP address and port. Inside, received and sent messages are captured into two binary files, msgs_recv.dat and msgs_sent.dat, like so:
```
message_capture/203.0.113.7:56072/msgs_recv.dat
message_capture/203.0.113.7:56072/msgs_sent.dat
```
Because the messages are raw binary dumps, included in this PR is a Python parsing tool to convert the binary files into human-readable JSON. This script has been placed on its own and out of the way in the new `contrib/message-capture` folder. Its usage is simple and easily discovered by the autogenerated `-h` option.
## Future Maintenance
I sympathize greatly with anyone who says "the best code is no code".
The future maintenance of this feature will be minimal. The logic to deserialize the payload of the p2p messages exists in our testing framework. As long as our testing framework works, so will this tool.
Additionally, I hope that the simplicity of this tool will mean that it gets used frequently, so that problems will be discovered and solved when they are small.
## FAQ
"Why not just use Wireshark"
Yes, Wireshark has the ability to filter and decode Bitcoin messages. However, the purpose of the message capture added in this PR is to assist with debugging, primarily for new developers looking to improve their knowledge of the Bitcoin Protocol. This drives the design in a different direction than Wireshark, in two different ways. First, this tool must be convenient and simple to use. Using an external tool, like Wireshark, requires setup and interpretation of the results. To a new user who doesn't necessarily know what to expect, this is unnecessary difficulty. This tool, on the other hand, "just works". Turn on the command line flag, run your node, run the script, read the JSON. Second, because this tool is being used for debugging, we want it to be as close to the true behavior of the node as possible. A lot can happen in the SocketHandler thread that would be missed by Wireshark.
Additionally, if we are to use Wireshark, we are at the mercy of whoever it maintaining the protocol in Wireshark, both as to it being accurate and recent. As can be seen by the **many** previous attempts to include Bitcoin in Wireshark (google "bitcoin dissector") this is easier said than done.
Lastly, I truly believe that this tool will be used significantly more by being included in the codebase. It's just that much more discoverable.
ACKs for top commit:
MarcoFalke:
re-ACK bff7c66e67 only some minor changes: 👚
jnewbery:
utACK bff7c66e67
theStack:
re-ACK bff7c66e67
Tree-SHA512: e59e3160422269221f70f98720b47842775781c247c064071d546c24fa7a35a0e5534e8baa4b4591a750d7eb16de6b4ecf54cbee6d193b261f4f104e28c15f47
Since these chainstates are:
1. Also vulnerable to the race condition described in the previous
commit
2. Documented as having similar semantics as m_active_chainstate
we should also protect them with ::cs_main.
b6aadcd5b4 build: Add -Werror=mismatched-tags (Hennadii Stepanov)
1485124291 Fix -Wmismatched-tags warnings (Hennadii Stepanov)
Pull request description:
Warnings were introduced in #20749:
```
./validation.h:43:1: warning: class 'CCheckpointData' was previously declared as a struct; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Wmismatched-tags]
class CCheckpointData;
^
./chainparams.h:24:8: note: previous use is here
struct CCheckpointData {
^
./validation.h:43:1: note: did you mean struct here?
class CCheckpointData;
^~~~~
struct
1 warning generated.
```
This change fixes AppVeyor build: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/37547435
ACKs for top commit:
glozow:
utACK b6aadcd5b4🚗
practicalswift:
cr ACK b6aadcd5b4: patch looks correct
Tree-SHA512: 3ac887ebdbf9a1ae33c1fd5381b3b8d83388ad557ddeb55013acd42bb9752a5bd009e3a0eed52644a023a7a0dda1c159277981af82f58fb0abfe60b84e01bf29
I wondered how one could enable debug logging with `-debug=<category>` for multiple categories. Found out solution is to specify that option multiple times for each wanted category.
This PR documents this behavior and uses the same wording for the same behavior of `-debugexclude=<category>` to make that also clear and stringent.
fa29272459 Remove redundant MakeUCharSpan wrappers (MarcoFalke)
faf4aa2f47 Remove CDataStream::Init in favor of C++11 member initialization (MarcoFalke)
fada14b948 Treat CDataStream bytes as uint8_t (MarcoFalke)
fa8bdb048e refactor: Drop CDataStream constructors in favor of one taking a Span of bytes (MarcoFalke)
faa96f841f Remove unused CDataStream methods (MarcoFalke)
Pull request description:
Using `uint8_t` for raw bytes has a style benefit:
* The signedness is clear from reading the code, as it does not depend on the architecture
Other clean-ups in this pull include:
* Remove unused methods
* Constructor is simplified with `Span`
* Remove `Init()` member in favor of C++11 member initialization
ACKs for top commit:
laanwj:
code review ACK fa29272459
theStack:
ACK fa29272459🍾
Tree-SHA512: 931ee28bd99843d7e894b48e90e1187ffb0278677c267044b3c0c255069d9bbd9298ab2e539b1002a30b543d240450eaec718ef4ee95a7fd4be0a295e926343f
dc8be12510 refactor: remove boost::thread_group usage (fanquake)
Pull request description:
Post #18710, there isn't much left using `boost::thread_group`, so should just be able to replace it with the standard library. This also removes the last use of `boost::thread_interrupted`.
After this change, last piece of Boost Thread we'd be using is `boost::shared_mutex`. See the commentary [here](https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696) as to why it may be non-trivial to swap that for `std::shared_mutex` in the near future.
Closes #17307
ACKs for top commit:
laanwj:
Code review re-ACK dc8be12510
MarcoFalke:
review ACK dc8be12510🔁
jonatack:
Non-expert code review ACK dc8be12510, also checked range-diff since last review and that local debug build is clean with gcc 10.2.1-6 on Debian
Tree-SHA512: 5510e2d760cce824234207dc86b1551ca8f21cbf3a2ce753c0254a0d03ffd83c94e449aec202fb7bd76e6fc64df783a6b70a736b0add9ece3734bb9c8ce8fc2f
67c9a83df1 style-only: Remove redundant sentence in ActivateBestChain comment (Carl Dong)
b8e95658d5 style-only: Make TestBlockValidity signature readable (Carl Dong)
0cdad75390 validation: Use accessible chainstate in ChainstateManager::ProcessNewBlock (Carl Dong)
ea4fed9021 validation: Use existing chainstate in ChainstateManager::ProcessNewBlockHeaders (Carl Dong)
e0dc305727 validation: Move LoadExternalBlockFile to CChainState (Carl Dong)
5f8cd7b3a5 validation: Remove global ::ActivateBestChain (Carl Dong)
2a696472a1 validation: Pass in chainstate to ::NotifyHeaderTip (Carl Dong)
9c300cc8b3 validation: Pass in chainstate to TestBlockValidity (Carl Dong)
0e17c833cd validation: Make CChainState.m_blockman public (Carl Dong)
d363d06bf7 validation: Pass in blockman to ContextualCheckBlockHeader (Carl Dong)
f11d11600d validation: Move GetLastCheckpoint to BlockManager (Carl Dong)
e4b95eefbc validation: Move GetSpendHeight to BlockManager (Carl Dong)
b026e318c3 validation: Move FindForkInGlobalIndex to BlockManager (Carl Dong)
3664a150ac validation: Remove global LookupBlockIndex (Carl Dong)
eae54e6e60 scripted-diff: Use BlockManager::LookupBlockIndex (Carl Dong)
15d20f40e1 validation: Move LookupBlockIndex to BlockManager (Carl Dong)
f92dc6557a validation: Guard the active_chainstate with cs_main (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
jnewbery:
utACK 67c9a83df1
laanwj:
re-ACK 67c9a83df1
ryanofsky:
Code review ACK 67c9a83df1. Changes since last review:
Tree-SHA512: 8744aba2dd57a40cd2fedca809b0fe24d771bc60da1bffde89601999384aa0df428057a86644a3f72fbeedbc8b04db6c4fd264ea0db2e73c279e5acc6d056cbf
74d23bf7fb rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN exception (Jarol Rodriguez)
Pull request description:
It is not documented in the `RPCHelpMan` of `sendrawtransaction` that if you attempt to send a transaction which already exists in a block, an `RPC_TRANSACTION_ALREADY_IN_CHAIN` exception will be raised. It is best to make developers aware of this so that it can be properly caught and avoid any headaches.
Closes #5638
ACKs for top commit:
jonatack:
ACK 74d23bf7fb
Tree-SHA512: d1d5fc242574377c8a76b4ef7b12239996424d8bee186533b5a8fe337bbeb3186e51dbdd28c5eafb982601e44e17b68a7f52db5dd7bc647429f6f95e2de289f6
c943282b5e validation: remove redundant check on pindex (jarolrod)
Pull request description:
This removes a redundant check on `pindex` being a `nullptr`. By the time we get to this step `pindex` is always a `nullptr` as the branch where it has been set would have already returned.
Closes #19223
ACKs for top commit:
Zero-1729:
re-ACK c943282
ajtowns:
ACK c943282b5e - code review only
MarcoFalke:
review ACK c943282b5e📨
theStack:
re-ACK c943282b5e
Tree-SHA512: d2dc58206be61d2897b0703ee93af67abed492a80e84ea03dcfbf7116833173da3bdafa18ff80422d5296729d5254d57cc1db03fdaf817c8f57c62c3abef673c
5d4597666d Rewrite OutputGroups to be clearer and to use scriptPubKeys (Andrew Chow)
f6b3052739 Explicitly filter out partial groups when we don't want them (Andrew Chow)
416d74fb16 Move OutputGroup positive only filtering into Insert (Andrew Chow)
d895e98b59 Move EligibleForSpending into GroupOutputs (Andrew Chow)
99b399aba5 Move fee setting of OutputGroup to Insert (Andrew Chow)
6148a8acda Move GroupOutputs into SelectCoinsMinConf (Andrew Chow)
2acad03657 Remove OutputGroup non-default constructors (Andrew Chow)
Pull request description:
Even after #17458, we still deal with setting fees of an `OutputGroup` and filtering the `OutputGroup` outside of the struct. We currently make all of the `OutputGroup`s in `SelectCoins` and then copy and modify them within each `SelectCoinsMinConf` scenario. This PR changes this to constructing the `OutputGroup`s within the `SelectCoinsMinConf` so that the scenario can be taken into account during the group construction. Furthermore, setting of fees and filtering for effective value is moved into `OutputGroup::Insert` itself so that we don't add undesirable outputs to an `OutputGroup` rather than deleting them afterwards.
To facilitate fee calculation and effective value filtering during `OutputGroup::Insert`, `OutputGroup` now takes the feerates in its constructor and computes the fees and effective value for each output during `Insert`.
While removing `OutputGroup`s in accordance with the `CoinEligibilityFilter` still requires creating the `OutputGroup`s first, we can do that within the function that makes them - `GroupOutput`s.
ACKs for top commit:
Xekyo:
Code review ACK: 5d4597666d
fjahr:
Code review ACK 5d4597666d
meshcollider:
Light utACK 5d4597666d
Tree-SHA512: 35965b6d49a87f4ebb366ec4f00aafaaf78e9282481ae2c9682b515a3a9f2cbcd3cd6e202fee29489d48fe7f3a7cede4270796f5e72bbaff76da647138fb3059
48c8a9b964 net_processing: log txrelay flag from version message (Anthony Towns)
98fab37ca0 net: use peer=N instead of from=N in debug log (Anthony Towns)
12302105bb net_processing: additional debug logging for ignored messages (Anthony Towns)
f7edea3b7c net: make debug logging conditional on -debug=net (Anthony Towns)
a410ae8cb0 net, net_processing: log disconnect reasons with -debug=net (Anthony Towns)
Pull request description:
A few changes to -debug=net logging:
* always log when disconnecting a peer
* only log various connection errors when -debug=net is enabled, since errors from random untrusted peers is completely expected
* log when ignoring a message due to violating protocol (primarily to make it easier to debug other implementations)
* use "peer=123" rather than "from 123" to make grepping logs a bit easier
* log the value of the bip-37 `fRelay` field in version messages both when sending and receiving a version message
ACKs for top commit:
jnewbery:
ACK 48c8a9b964
MarcoFalke:
re-ACK 48c8a9b964 only change is rebase 🚓
practicalswift:
re-ACK 48c8a9b964
Tree-SHA512: 6ac530d883dffc4fd7fe20b1dc5ebb5394374c9b499aa7a253eb4a3a660d8901edd72e5ad21ce4a2bf71df25e8f142087755f9756f3497f564ef453a7e9246c1
5d1f260713 Improve gui/src/qt README.md (Jarol Rodriguez)
Pull request description:
**Master/Before:** [Render of Master](https://github.com/bitcoin-core/gui/blob/master/src/qt/README.md)
**PR/After:** [Render of PR](5d1f260713/src/qt/README.md)
**Changes:**
The README.md found in `gui/src/qt` seems to not have gotten any love in a while. This PR fixes some grammatical errors, makes it easier to follow, and modernizes the logic of using Qt Creator.
1. Makes several sections more informative
2. Directories under `Files and Directories` now end with a forward slash denoting that they are a directory
3. Modernize the Qt Creator Logic for the current setup flow
4. Add UNIX Qt Creator Setup Instructions (Ubuntu & Debian)
ACKs for top commit:
RandyMcMillan:
ACK 5d1f260👍🏼
jonatack:
ACK 5d1f260713
Tree-SHA512: bd5cc24b95460f34a1efa489a6acc10d7632c84eabdcdc5729ef077d8303cf037ed664aae033af8883252433ea0999d8ec7d92e8b03d03a873d32b041a94f813
