backport: Merge bitcoin#20773, 22147, 21800, #5751
Closed
+2,178
−311
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request has conflicts, please rebase. |
vijaydasmp
changed the title
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
bp22_20
branch
10 times, most recently
from
58672b1
to
87f7113
Compare
…d CSerializedNetMsg 51e9393 refactor: s/command/msg_type/ in CNetMsgMaker and CSerializedNetMsg (Sebastian Falbesoner) Pull request description: Follow-up PR for bitcoin#18533 -- another small step towards getting rid of the confusing "command" terminology. Also see PR bitcoin#18610 which tackled the functional tests. ACKs for top commit: MarcoFalke: ACK 51e9393 Tree-SHA512: bb6f05a7be6823d5c4eab1d05b31fee944e700946827ad9425d59a3957fd879776c88c606319cbe9832d9451b275baedf913b71429ea3e01e4e82bf2d419e819
bff7c66 Add documentation to contrib folder (Troy Giorshev) 381f77b Add Message Capture Test (Troy Giorshev) e4f378a Add capture parser (Troy Giorshev) 4d1a582 Call CaptureMessage at appropriate locations (Troy Giorshev) f2a77ff Add CaptureMessage (Troy Giorshev) dbf779d 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 bff7c66 only some minor changes: 👚 jnewbery: utACK bff7c66 theStack: re-ACK bff7c66 Tree-SHA512: e59e3160422269221f70f98720b47842775781c247c064071d546c24fa7a35a0e5534e8baa4b4591a750d7eb16de6b4ecf54cbee6d193b261f4f104e28c15f47
…ct reason 1f44958 test: add `bad-txns-prevout-null` test to mempool_accept.py (Sebastian Falbesoner) aa0a5bb test: add `bad-txns-prevout-null` test case to invalid_txs.py (Sebastian Falbesoner) Pull request description: This simple PR adds missing tests for the reject reason `bad-txns-prevout-null`, which is thrown in the function `CheckTransaction()`: https://github.com/bitcoin/bitcoin/blob/a62fc35a150da584d39d7cd01ade14bbb5002fb9/src/consensus/tx_check.cpp#L52-L54 Basically this condition is met for non-coinbase transactions (the code snippet above only hits if `!tx.IsCoinBase()`) with coinbase-like outpoints, i.e. hash=0, n=0xffffffff. Can be tested by running the functional tests `feature_block.py`, `p2p_invalid_tx.py` and `mempool_accept.py`. Not sure if the redundancy in the tests is desired (I guess it would make sense if the mempool acceptance test also makes use of the invalid_txs templates?). ACKs for top commit: rajarshimaitra: tACK bitcoin@1f44958 brunoerg: tACK 1f44958 kristapsk: ACK 1f44958, code looks correct and all tests pass. Tree-SHA512: 2d4f940a6ac8e0d80d2670c9e1111cbf43ae6ac62809a2ccf17cffee9a41d387ea4d889ee300eb4a407c055b13bfa5d37102a32ed59964a9b6950bd907ba7204
489ebb7 wallet: make chain optional for CWallet::Create (Ivan Metlushko) d73ae93 CWallet::Create move chain init message up into calling code (Ivan Metlushko) 44c430f refactor: Add CWallet:::AttachChain method (Russell Yanofsky) e2a47ce refactor: move first run detection to client code (Ivan Metlushko) Pull request description: This is a followup for bitcoin#20365 (comment) First part of a refactoring with overall goal to simplify `CWallet` and de-duplicate code with `wallettool` **Rationale**: split `CWallet::Create` and create `CWallet::AttachChain`. `CWallet::AttachChain` takes chain as first parameter on purpose. In future I suggest we can remove `chain` from `CWallet` constructor. The second commit is based on be164f9 from bitcoin#15719 (thanks ryanofsky) cc ryanofsky achow101 ACKs for top commit: ryanofsky: Code review ACK 489ebb7. Only changes since last review were adding a const variable declaration, and implementing suggestion not to move feerate option checks to AttachChain. Thanks for updates and fast responses! Tree-SHA512: 00235abfe1b00874c56c449adcab8a36582424abb9ba27440bf750af8f3f217b68c11ca74eb30f78a2109ad1d9009315480effc78345e16a3074a1b5d8128721
30aee2d tests: Add test for compact block HB selection (Pieter Wuille) 6efbcec Protect last outbound HB compact block peer (Suhas Daftuar) Pull request description: If all our high-bandwidth compact block serving peers (BIP 152) stall block download, then we can be denied a block for (potentially) a long time. As inbound connections are much more likely to be adversarial than outbound connections, mitigate this risk by never removing our last outbound HB peer if it would be replaced by an inbound. ACKs for top commit: achow101: ACK 30aee2d ariard: Code ACK 30aee2d jonatack: ACK 30aee2d Tree-SHA512: 5c6c9326e3667b97e0864c371ae2174d2be9054dad479f4366127b9cd3ac60ffa01ec9707b16ef29cac122db6916cf56fd9985733390017134ace483278921d5
…time spent waiting b9e76f1 rpc: Add test for -rpcwaittimeout (Christian Decker) f76cb10 rpc: Prefix rpcwaittimeout error with details on its nature (Christian Decker) c490e17 doc: Add release notes for the `-rpcwaittimeout` cli parameter (Christian Decker) a7fcc8e rpc: Add a `-rpcwaittimeout` parameter to limit time spent waiting (Christian Decker) Pull request description: Adds a new numeric `-rpcwaittimeout` that can be used to limit the time we spend waiting on the RPC server to appear. This is used by downstream projects to provide a bit of slack when `bitcoind`s RPC interface is not available right away. This makes the `-rpcwait` argument more useful, since we can now limit how long we'll ultimately wait, before potentially giving up and reporting an error to the caller. It was discussed in the context of the BTCPayServer wanting to have c-lightning wait for the RPC interface to become available but still have the option of giving up eventually ([4355]). I checked with laanwj whether this is already possible ([comment]), and whether this would be a welcome change. Initially I intended to repurpose the (optional) argument to `-rpcwait`, however I decided against it since it would potentially break existing configurations, using things like `rpcwait=1`, or `rpcwait=true` (the former would have an unintended short timeout, when old behavior was to wait indefinitely). ~Due to its simplicity I didn't implement a test for it yet, but if that's desired I can provide one.~ Test was added during reviews. [4355]: ElementsProject/lightning#4355 [comment]: ElementsProject/lightning#4355 (comment) ACKs for top commit: laanwj: Code review ACK b9e76f1 promag: ACK b9e76f1. Tree-SHA512: 3cd6728038ec7ca7c35c2e7ccb213bfbe963f99a49bb48bbc1e511c4dd23d9957c04f9af1f8ec57120e47b26eaf580b46817b099d5fc5083c98da7aa92db8638
…limits for packages accf3d5 [test] mempool package ancestor/descendant limits (glozow) 2b6b26e [test] parameterizable fee for make_chain and create_child_with_parents (glozow) 313c09f [test] helper function to increase transaction weight (glozow) f8253d6 extract/rename helper functions from rpc_packages.py (glozow) 3cd663a [policy] ancestor/descendant limits for packages (glozow) c6e016a [mempool] check ancestor/descendant limits for packages (glozow) f551841 [refactor] pass size/count instead of entry to CalculateAncestorsAndCheckLimits (glozow) 97dd1c7 MOVEONLY: add helper function for calculating ancestors and checking limits (glozow) f95bbf5 misc package validation doc improvements (glozow) Pull request description: This PR implements a function to calculate mempool ancestors for a package and enforces ancestor/descendant limits on them as a whole. It reuses a portion of `CalculateMemPoolAncestors()`; there's also a small refactor to move the reused code into a generic helper function. Instead of calculating ancestors and descendants on every single transaction in the package and their ancestors, we use a "worst case" heuristic, treating every transaction in the package as each other's ancestor and descendant. This may overestimate everyone's counts, but is still pretty accurate in the our main package use cases, in which at least one of the transactions in the package is directly related to all the others (e.g. 1 parent + 1 child, multiple parents with 1 child, or chains). Note on Terminology: While "package" is often used to describe groups of related transactions _within_ the mempool, here, I only use package to mean the group of not-in-mempool transactions we are currently validating. #### Motivation It would be a potential DoS vector to allow submission of packages to mempool without a proper guard for mempool ancestors/descendants. In general, the purpose of mempool ancestor/descendant limits is to limit the computational complexity of dealing with families during removals and additions. We want to be able to validate multiple transactions on top of the mempool, but also avoid these scenarios: - We underestimate the ancestors/descendants during package validation and end up with extremely complex families in our mempool (potentially a DoS vector). - We expend an unreasonable amount of resources calculating everyone's ancestors and descendants during package validation. ACKs for top commit: JeremyRubin: utACK accf3d5 ariard: ACK accf3d5. Tree-SHA512: 0d18ce4b77398fe872e0b7c2cc66d3aac2135e561b64029584339e1f4de2a6a16ebab3dd5784f376e119cbafc4d50168b28d3bd95d0b3d01158714ade2e3624d
vijaydasmp
changed the title
…s cleanups/improvements bdb8b9a test: doc: improve doc for `from_hex` helper (mention `to_hex` alternative) (Sebastian Falbesoner) 1914054 scripted-diff: test: rename `FromHex` to `from_hex` (Sebastian Falbesoner) a79396f test: remove `ToHex` helper, use .serialize().hex() instead (Sebastian Falbesoner) 2ce7b47 test: introduce `tx_from_hex` helper for tx deserialization (Sebastian Falbesoner) Pull request description: There are still many functional tests that perform conversions from a hex-string to a message object (deserialization) manually. This PR identifies all those instances and replaces them with a newly introduced helper `tx_from_hex`. Instances were found via * `git grep "deserialize.*BytesIO"` and some of them manually, when it were not one-liners. Further, the helper `ToHex` was removed and simply replaced by `.serialize().hex()`, since now both variants are in use (sometimes even within the same test) and using the helper doesn't really have an advantage in readability. (see discussion bitcoin#22257 (comment)) ACKs for top commit: MarcoFalke: review re-ACK bdb8b9a 😁 Tree-SHA512: e25d7dc85918de1d6755a5cea65471b07a743204c20ad1c2f71ff07ef48cc1b9ad3fe5f515c1efaba2b2e3d89384e7980380c5d81895f9826e2046808cd3266e
…etpeerinfo 343dc47 test: add test for high-bandwidth mode states in getpeerinfo (Sebastian Falbesoner) dab6583 doc: release note for new getpeerinfo fields "bip152_hb_{from,to}" (Sebastian Falbesoner) a7ed00f rpc: expose high-bandwidth mode states via getpeerinfo (Sebastian Falbesoner) 30bc8fa net: save high-bandwidth mode states in CNodeStats (Sebastian Falbesoner) Pull request description: Fixes bitcoin#19676, "_For every peer expose through getpeerinfo RPC whether or not we selected them as HB peers, and whether or not they selected us as HB peers._" See [BIP152](https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki), in particular the [protocol flow diagram](https://github.com/bitcoin/bips/raw/master/bip-0152/protocol-flow.png). The newly introduced states are changed on the following places in the code: * on reception of a `SENDCMPCT` message with valid version, the field `m_highbandwidth_from` is changed depending on the first integer parameter in the message (1=high bandwidth, 0=low bandwidth), i.e. it just mirrors the field `CNodeState.fPreferHeaderAndIDs`. * after adding a `SENDCMPCT` message to the send queue, the field `m_highbandwidth_to` is changed depending on how the first integer parameter is set (same as above) Note that after receiving `VERACK`, the node also sends `SENDCMPCT`, but that is only to announce the preferred version and never selects high-bandwidth mode, hence there is no need to change the state variables there, which are initialized to `false` anyways. ACKs for top commit: naumenkogs: reACK 343dc47 jonatack: re-ACK 343dc47 per `git range-diff 7ea6499 4df1d12 343dc47` Tree-SHA512: f4999e6a935266812c2259a9b5dc459710037d3c9e938006d282557cc225e56128f72965faffb207fc60c6531fab1206db976dd8729a69e8ca29d4835317b99f
…h testmempoolaccept 13650fe [policy] detect unsorted packages (glozow) 9ef643e [doc] add release note for package testmempoolaccept (glozow) c4259f4 [test] functional test for packages in RPCs (glozow) 9ede34a [rpc] allow multiple txns in testmempoolaccept (glozow) ae8e6df [policy] limit package sizes (glozow) c9e1a26 [fuzz] add ProcessNewPackage call in tx_pool fuzzer (glozow) 363e3d9 [test] unit tests for ProcessNewPackage (glozow) cd9a11a [test] make submit optional in CreateValidMempoolTransaction (glozow) 2ef1879 [validation] package validation for test accepts (glozow) 578148d [validation] explicit Success/Failure ctors for MempoolAcceptResult (glozow) b88d77a [policy] Define packages (glozow) 249f43f [refactor] add option to disable RBF (glozow) 897e348 [coins/mempool] extend CCoinsViewMemPool to track temporary coins (glozow) 42cf8b2 [validation] make CheckSequenceLocks context-free (glozow) Pull request description: This PR enables validation dry-runs of packages through the `testmempoolaccept` RPC. The expectation is that the results returned from `testmempoolaccept` are what you'd get from test-then-submitting each transaction individually, in that order (this means the package is expected to be sorted in topological order, for now at least). The validation is also atomic: in the case of failure, it immediately halts and may return "unfinished" `MempoolAcceptResult`s for transactions that weren't fully validated. The API for 1 transaction stays the same. **Motivation:** - This allows you to test validity for transaction chains (e.g. with multiple spending paths and where you don't want to broadcast yet); closes bitcoin#18480. - It's also a first step towards package validation in a minimally invasive way. - The RPC commit happens to close bitcoin#21074 by clarifying the "allowed" key. There are a few added restrictions on the packages, mostly to simplify the logic for areas that aren't critical to main package use cases: - No package can have conflicts, i.e. none of them can spend the same inputs, even if it would be a valid BIP125 replacement. - The package cannot conflict with the mempool, i.e. RBF is disabled. - The total count of the package cannot exceed 25 (the default descendant count limit), and total size cannot exceed 101KvB (the default descendant size limit). If you're looking for review comments and github isn't loading them, I have a gist compiling some topics of discussion [here](https://gist.github.com/glozow/c3acaf161c95bba491fce31585b2aaf7) ACKs for top commit: laanwj: Code review re-ACK 13650fe jnewbery: Code review ACK 13650fe ariard: ACK 13650fe Tree-SHA512: 8c5cbfa91a6c714e1c8710bb281d5ff1c5af36741872a7c5df6b24874d6272b4a09f816cb8a4c7de33ef8e1c2a2c252c0df5105b7802f70bc6ff821ed7cc1a2f
|
This pull request has conflicts, please rebase. |
vijaydasmp
changed the title
vijaydasmp
changed the title
vijaydasmp
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bitcoin Backports