backport: Merge bitcoin#21090,22006 by vijaydasmp · Pull Request #5662 · dashpay/dash

vijaydasmp · 2023-11-01T17:06:02Z

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using -segwitheight=-1 (Dhruv Mehta)

Pull request description:

Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862

Removing RewindBlockIndex() in bitcoin#21009 allows the following:

removal of tests using segwitheight=-1 in p2p_segwit.py.
move test_upgrade_after_activation() out of p2p_segwit.py reducing runtime
in turn, that allows us to drop support for -segwitheight=-1, which is only supported for that test.
that allows us to always set NODE_WITNESS in our local services. The only reason we don't do that is to support -segwitheight=-1.
that in turn allows us to drop all of the GetLocalServices() & NODE_WITNESS checks inside net_processing.cpp, since our local services would always include NODE_WITNESS

ACKs for top commit:
mzumsande:
Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647

Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

Please remove the italicized help prompts before submitting or merging

Provide a general summary of your changes in the Title above

Pull requests without a rationale and clear improvement may be closed
immediately.

Please provide clear motivation for your patch and explain how it improves
Dash Core user experience or Dash Core developer experience
significantly:

Any test improvements or new tests that improve coverage are always welcome.
All other changes should have accompanying unit tests (see src/test/) or
functional tests (see test/). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Dash Core, if possible.

Issue being fixed or feature implemented

Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.

What was done?

Describe your changes in detail

How Has This Been Tested?

Please describe in detail how you tested your changes.

Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.

Breaking Changes

Please describe any breaking changes your code introduces

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta) Pull request description: Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862 Removing `RewindBlockIndex()` in bitcoin#21009 allows the following: - removal of tests using `segwitheight=-1` in `p2p_segwit.py`. - move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime - in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test. - that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`. - that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS` ACKs for top commit: mzumsande: Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647 Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

…User-Space, Statically Defined Tracing (USDT) 8f37f5c tracing: Tracepoint for connected blocks (0xb10c) 4224dec tracing: Tracepoints for in- and outbound P2P msgs (0xb10c) 469b71a doc: document systemtap dependency (0xb10c) 84ace9a doc: Add initial USDT documentation (0xb10c) Pull request description: This PR adds documentation for User-Space, Statically Defined Tracing (USDT) as well as three tracepoints (including documentation and usage examples). ## Context The `TRACEx` macros for tracepoints and build system changes for USDT were merged in bitcoin#19866 earlier this year. Issue bitcoin#20981 contains discussion about potential tracepoints and guidelines for adding them (also documented with this PR). USDT was a topic in a [core-dev-meeting discussion](https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2021/bitcoin-core-dev.2021-01-21-19.00.moin.txt) on 21st Jan, 2021. - [collabora.com: An eBPF overview, part 1: Introduction](https://www.collabora.com/news-and-blog/blog/2019/04/05/an-ebpf-overview-part-1-introduction/) - [collabora.com: An eBPF overview, part 2: Machine & bytecode](https://www.collabora.com/news-and-blog/blog/2019/04/15/an-ebpf-overview-part-2-machine-and-bytecode/) - [Brendan D. Gregg's blog posts, and book on on eBPF](http://www.brendangregg.com/) - [Brendan D. Gregg: Linux bcc/BPF Node.js USDT Tracing](http://www.brendangregg.com/blog/2016-10-12/linux-bcc-nodejs-usdt.html) ## USDT? Stablecoin? User-space, Statically Defined Tracing (USDT) allows for more observability during development, debugging, code review, and production usage. The tracepoints make it possible to keep track of custom statistics and enable detailed monitoring of otherwise hidden internals and have little to no performance impact when unused. Linux kernels (4.x or newer) can hook into the tracepoints and execute [eBPF] programs in a kernel VM once the tracepoint is called. This PR includes, for example, tracepoints for in- and outbound P2P messages. ``` USDT and eBPF Overview ====================== ┌──────────────────┐ ┌──────────────┐ │ tracing script │ │ bitcoind │ │==================│ 2. │==============│ │ eBPF │ tracing │ hooks │ │ │ code │ logic │ into┌─┤►tracepoint 1─┼───┐ 3. └────┬───┴──▲──────┘ ├─┤►tracepoint 2 │ │ pass args 1. │ │ 4. │ │ ... │ │ to eBPF User compiles │ │ pass data to │ └──────────────┘ │ program Space & loads │ │ tracing script │ │ ─────────────────┼──────┼─────────────────┼────────────────────┼─── Kernel │ │ │ │ Space ┌──┬─▼──────┴─────────────────┴────────────┐ │ │ │ eBPF program │◄──────┘ │ └───────────────────────────────────────┤ │ eBPF kernel Virtual Machine (sandboxed) │ └──────────────────────────────────────────┘ 1. The tracing script compiles the eBPF code and loads the eBFP program into a kernel VM 2. The eBPF program hooks into one or more tracepoints 3. When the tracepoint is called, the arguments are passed to the eBPF program 4. The eBPF program processes the arguments and returns data to the tracing script ``` The two main [eBPF] front-ends with support for USDT are [bpftrace] an [BPF Compiler Collection (BCC)]. BCC is used for complex tools and daemons and `bpftrace` is preferred for one-liners and shorter scripts. Example tracing scripts for both are provided with this PR. [eBPF]: https://ebpf.io/ [bpftrace]: https://github.com/iovisor/bpftrace [BPF Compiler Collection (BCC)]: https://github.com/iovisor/bcc This PR adds three tracepoints: - `net:inbound_message` - `net:outbound_message` - `valildation:block_connected` See `doc/tracing.md` and `contrib/tracing/` for documentation and example tracing scripts. ## Open Questions (Not in scope for this PR) - How to use these tracepoints under macOS? - Release builds with USDT support? - Should and can the tracepoints be automatically tested? ## Todo (before undraft) - [x] bcc example showing how to read raw P2P messages up to 32kb - [x] document that you need `sys/sdt.h` from `systemtap` for USDT support in Bitcoin Core (`apt install systemtap-sdt-dev` on debian-like). See bitcoin@933ab8a - [ ] release notes? ACKs for top commit: laanwj: re-ACK 8f37f5c jb55: ACK 8f37f5c Tree-SHA512: a92a8a2dfcd28465f58a6e5f50d39486817ef5f51214ec40bdb02a6843b9c08ea154fadb31558825ff3a4687477b90f2a5da5d6451989eef978e128a264c289d

fadfd84 test: Remove unused connect_nodes_bi (MarcoFalke) fa3b9ee scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke) faaee1e test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke) 1111bb9 test: Reformat python imports to aid scripted diff (MarcoFalke) Pull request description: By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side). This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times. Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests. Historic background: `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug dashpay#5113 and dashpay#5138, which has long been fixed in dashpay#5157 and dashpay#5662. ACKs for top commit: laanwj: ACK fadfd84 jonasschnelli: utACK fadfd84 - more of less a cleanup PR. promag: Tested ACK fadfd84, ran extended tests. Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24

github-actions · 2024-01-28T04:57:47Z

This pull request has conflicts, please rebase.

vijaydasmp · 2023-11-01T17:06:02Z

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using -segwitheight=-1 (Dhruv Mehta)

Pull request description:

Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862

Removing RewindBlockIndex() in bitcoin#21009 allows the following:

removal of tests using segwitheight=-1 in p2p_segwit.py.
move test_upgrade_after_activation() out of p2p_segwit.py reducing runtime
in turn, that allows us to drop support for -segwitheight=-1, which is only supported for that test.
that allows us to always set NODE_WITNESS in our local services. The only reason we don't do that is to support -segwitheight=-1.
that in turn allows us to drop all of the GetLocalServices() & NODE_WITNESS checks inside net_processing.cpp, since our local services would always include NODE_WITNESS

ACKs for top commit:
mzumsande:
Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647

Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

Please remove the italicized help prompts before submitting or merging

Provide a general summary of your changes in the Title above

Pull requests without a rationale and clear improvement may be closed
immediately.

Please provide clear motivation for your patch and explain how it improves
Dash Core user experience or Dash Core developer experience
significantly:

Any test improvements or new tests that improve coverage are always welcome.
All other changes should have accompanying unit tests (see src/test/) or
functional tests (see test/). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Dash Core, if possible.

Issue being fixed or feature implemented

Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.

What was done?

Describe your changes in detail

How Has This Been Tested?

Please describe in detail how you tested your changes.

Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.

Breaking Changes

Please describe any breaking changes your code introduces

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta) Pull request description: Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862 Removing `RewindBlockIndex()` in bitcoin#21009 allows the following: - removal of tests using `segwitheight=-1` in `p2p_segwit.py`. - move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime - in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test. - that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`. - that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS` ACKs for top commit: mzumsande: Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647 Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

…User-Space, Statically Defined Tracing (USDT) 8f37f5c tracing: Tracepoint for connected blocks (0xb10c) 4224dec tracing: Tracepoints for in- and outbound P2P msgs (0xb10c) 469b71a doc: document systemtap dependency (0xb10c) 84ace9a doc: Add initial USDT documentation (0xb10c) Pull request description: This PR adds documentation for User-Space, Statically Defined Tracing (USDT) as well as three tracepoints (including documentation and usage examples). ## Context The `TRACEx` macros for tracepoints and build system changes for USDT were merged in bitcoin#19866 earlier this year. Issue bitcoin#20981 contains discussion about potential tracepoints and guidelines for adding them (also documented with this PR). USDT was a topic in a [core-dev-meeting discussion](https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2021/bitcoin-core-dev.2021-01-21-19.00.moin.txt) on 21st Jan, 2021. - [collabora.com: An eBPF overview, part 1: Introduction](https://www.collabora.com/news-and-blog/blog/2019/04/05/an-ebpf-overview-part-1-introduction/) - [collabora.com: An eBPF overview, part 2: Machine & bytecode](https://www.collabora.com/news-and-blog/blog/2019/04/15/an-ebpf-overview-part-2-machine-and-bytecode/) - [Brendan D. Gregg's blog posts, and book on on eBPF](http://www.brendangregg.com/) - [Brendan D. Gregg: Linux bcc/BPF Node.js USDT Tracing](http://www.brendangregg.com/blog/2016-10-12/linux-bcc-nodejs-usdt.html) ## USDT? Stablecoin? User-space, Statically Defined Tracing (USDT) allows for more observability during development, debugging, code review, and production usage. The tracepoints make it possible to keep track of custom statistics and enable detailed monitoring of otherwise hidden internals and have little to no performance impact when unused. Linux kernels (4.x or newer) can hook into the tracepoints and execute [eBPF] programs in a kernel VM once the tracepoint is called. This PR includes, for example, tracepoints for in- and outbound P2P messages. ``` USDT and eBPF Overview ====================== ┌──────────────────┐ ┌──────────────┐ │ tracing script │ │ bitcoind │ │==================│ 2. │==============│ │ eBPF │ tracing │ hooks │ │ │ code │ logic │ into┌─┤►tracepoint 1─┼───┐ 3. └────┬───┴──▲──────┘ ├─┤►tracepoint 2 │ │ pass args 1. │ │ 4. │ │ ... │ │ to eBPF User compiles │ │ pass data to │ └──────────────┘ │ program Space & loads │ │ tracing script │ │ ─────────────────┼──────┼─────────────────┼────────────────────┼─── Kernel │ │ │ │ Space ┌──┬─▼──────┴─────────────────┴────────────┐ │ │ │ eBPF program │◄──────┘ │ └───────────────────────────────────────┤ │ eBPF kernel Virtual Machine (sandboxed) │ └──────────────────────────────────────────┘ 1. The tracing script compiles the eBPF code and loads the eBFP program into a kernel VM 2. The eBPF program hooks into one or more tracepoints 3. When the tracepoint is called, the arguments are passed to the eBPF program 4. The eBPF program processes the arguments and returns data to the tracing script ``` The two main [eBPF] front-ends with support for USDT are [bpftrace] an [BPF Compiler Collection (BCC)]. BCC is used for complex tools and daemons and `bpftrace` is preferred for one-liners and shorter scripts. Example tracing scripts for both are provided with this PR. [eBPF]: https://ebpf.io/ [bpftrace]: https://github.com/iovisor/bpftrace [BPF Compiler Collection (BCC)]: https://github.com/iovisor/bcc This PR adds three tracepoints: - `net:inbound_message` - `net:outbound_message` - `valildation:block_connected` See `doc/tracing.md` and `contrib/tracing/` for documentation and example tracing scripts. ## Open Questions (Not in scope for this PR) - How to use these tracepoints under macOS? - Release builds with USDT support? - Should and can the tracepoints be automatically tested? ## Todo (before undraft) - [x] bcc example showing how to read raw P2P messages up to 32kb - [x] document that you need `sys/sdt.h` from `systemtap` for USDT support in Bitcoin Core (`apt install systemtap-sdt-dev` on debian-like). See bitcoin@933ab8a - [ ] release notes? ACKs for top commit: laanwj: re-ACK 8f37f5c jb55: ACK 8f37f5c Tree-SHA512: a92a8a2dfcd28465f58a6e5f50d39486817ef5f51214ec40bdb02a6843b9c08ea154fadb31558825ff3a4687477b90f2a5da5d6451989eef978e128a264c289d

fadfd84 test: Remove unused connect_nodes_bi (MarcoFalke) fa3b9ee scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke) faaee1e test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke) 1111bb9 test: Reformat python imports to aid scripted diff (MarcoFalke) Pull request description: By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side). This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times. Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests. Historic background: `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug dashpay#5113 and dashpay#5138, which has long been fixed in dashpay#5157 and dashpay#5662. ACKs for top commit: laanwj: ACK fadfd84 jonasschnelli: utACK fadfd84 - more of less a cleanup PR. promag: Tested ACK fadfd84, ran extended tests. Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24

github-actions · 2024-01-28T04:57:47Z

This pull request has conflicts, please rebase.

vijaydasmp · 2023-11-01T17:06:02Z

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using -segwitheight=-1 (Dhruv Mehta)

Pull request description:

Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862

Removing RewindBlockIndex() in bitcoin#21009 allows the following:

removal of tests using segwitheight=-1 in p2p_segwit.py.
move test_upgrade_after_activation() out of p2p_segwit.py reducing runtime
in turn, that allows us to drop support for -segwitheight=-1, which is only supported for that test.
that allows us to always set NODE_WITNESS in our local services. The only reason we don't do that is to support -segwitheight=-1.
that in turn allows us to drop all of the GetLocalServices() & NODE_WITNESS checks inside net_processing.cpp, since our local services would always include NODE_WITNESS

ACKs for top commit:
mzumsande:
Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647

Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

Please remove the italicized help prompts before submitting or merging

Provide a general summary of your changes in the Title above

Pull requests without a rationale and clear improvement may be closed
immediately.

Please provide clear motivation for your patch and explain how it improves
Dash Core user experience or Dash Core developer experience
significantly:

Any test improvements or new tests that improve coverage are always welcome.
All other changes should have accompanying unit tests (see src/test/) or
functional tests (see test/). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Dash Core, if possible.

Issue being fixed or feature implemented

Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.

What was done?

Describe your changes in detail

How Has This Been Tested?

Please describe in detail how you tested your changes.

Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.

Breaking Changes

Please describe any breaking changes your code introduces

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta) Pull request description: Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862 Removing `RewindBlockIndex()` in bitcoin#21009 allows the following: - removal of tests using `segwitheight=-1` in `p2p_segwit.py`. - move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime - in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test. - that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`. - that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS` ACKs for top commit: mzumsande: Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647 Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

…User-Space, Statically Defined Tracing (USDT) 8f37f5c tracing: Tracepoint for connected blocks (0xb10c) 4224dec tracing: Tracepoints for in- and outbound P2P msgs (0xb10c) 469b71a doc: document systemtap dependency (0xb10c) 84ace9a doc: Add initial USDT documentation (0xb10c) Pull request description: This PR adds documentation for User-Space, Statically Defined Tracing (USDT) as well as three tracepoints (including documentation and usage examples). ## Context The `TRACEx` macros for tracepoints and build system changes for USDT were merged in bitcoin#19866 earlier this year. Issue bitcoin#20981 contains discussion about potential tracepoints and guidelines for adding them (also documented with this PR). USDT was a topic in a [core-dev-meeting discussion](https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2021/bitcoin-core-dev.2021-01-21-19.00.moin.txt) on 21st Jan, 2021. - [collabora.com: An eBPF overview, part 1: Introduction](https://www.collabora.com/news-and-blog/blog/2019/04/05/an-ebpf-overview-part-1-introduction/) - [collabora.com: An eBPF overview, part 2: Machine & bytecode](https://www.collabora.com/news-and-blog/blog/2019/04/15/an-ebpf-overview-part-2-machine-and-bytecode/) - [Brendan D. Gregg's blog posts, and book on on eBPF](http://www.brendangregg.com/) - [Brendan D. Gregg: Linux bcc/BPF Node.js USDT Tracing](http://www.brendangregg.com/blog/2016-10-12/linux-bcc-nodejs-usdt.html) ## USDT? Stablecoin? User-space, Statically Defined Tracing (USDT) allows for more observability during development, debugging, code review, and production usage. The tracepoints make it possible to keep track of custom statistics and enable detailed monitoring of otherwise hidden internals and have little to no performance impact when unused. Linux kernels (4.x or newer) can hook into the tracepoints and execute [eBPF] programs in a kernel VM once the tracepoint is called. This PR includes, for example, tracepoints for in- and outbound P2P messages. ``` USDT and eBPF Overview ====================== ┌──────────────────┐ ┌──────────────┐ │ tracing script │ │ bitcoind │ │==================│ 2. │==============│ │ eBPF │ tracing │ hooks │ │ │ code │ logic │ into┌─┤►tracepoint 1─┼───┐ 3. └────┬───┴──▲──────┘ ├─┤►tracepoint 2 │ │ pass args 1. │ │ 4. │ │ ... │ │ to eBPF User compiles │ │ pass data to │ └──────────────┘ │ program Space & loads │ │ tracing script │ │ ─────────────────┼──────┼─────────────────┼────────────────────┼─── Kernel │ │ │ │ Space ┌──┬─▼──────┴─────────────────┴────────────┐ │ │ │ eBPF program │◄──────┘ │ └───────────────────────────────────────┤ │ eBPF kernel Virtual Machine (sandboxed) │ └──────────────────────────────────────────┘ 1. The tracing script compiles the eBPF code and loads the eBFP program into a kernel VM 2. The eBPF program hooks into one or more tracepoints 3. When the tracepoint is called, the arguments are passed to the eBPF program 4. The eBPF program processes the arguments and returns data to the tracing script ``` The two main [eBPF] front-ends with support for USDT are [bpftrace] an [BPF Compiler Collection (BCC)]. BCC is used for complex tools and daemons and `bpftrace` is preferred for one-liners and shorter scripts. Example tracing scripts for both are provided with this PR. [eBPF]: https://ebpf.io/ [bpftrace]: https://github.com/iovisor/bpftrace [BPF Compiler Collection (BCC)]: https://github.com/iovisor/bcc This PR adds three tracepoints: - `net:inbound_message` - `net:outbound_message` - `valildation:block_connected` See `doc/tracing.md` and `contrib/tracing/` for documentation and example tracing scripts. ## Open Questions (Not in scope for this PR) - How to use these tracepoints under macOS? - Release builds with USDT support? - Should and can the tracepoints be automatically tested? ## Todo (before undraft) - [x] bcc example showing how to read raw P2P messages up to 32kb - [x] document that you need `sys/sdt.h` from `systemtap` for USDT support in Bitcoin Core (`apt install systemtap-sdt-dev` on debian-like). See bitcoin@933ab8a - [ ] release notes? ACKs for top commit: laanwj: re-ACK 8f37f5c jb55: ACK 8f37f5c Tree-SHA512: a92a8a2dfcd28465f58a6e5f50d39486817ef5f51214ec40bdb02a6843b9c08ea154fadb31558825ff3a4687477b90f2a5da5d6451989eef978e128a264c289d

fadfd84 test: Remove unused connect_nodes_bi (MarcoFalke) fa3b9ee scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke) faaee1e test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke) 1111bb9 test: Reformat python imports to aid scripted diff (MarcoFalke) Pull request description: By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side). This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times. Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests. Historic background: `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug dashpay#5113 and dashpay#5138, which has long been fixed in dashpay#5157 and dashpay#5662. ACKs for top commit: laanwj: ACK fadfd84 jonasschnelli: utACK fadfd84 - more of less a cleanup PR. promag: Tested ACK fadfd84, ran extended tests. Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24

github-actions · 2024-01-28T04:57:47Z

This pull request has conflicts, please rebase.

vijaydasmp · 2023-11-01T17:06:02Z

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using -segwitheight=-1 (Dhruv Mehta)

Pull request description:

Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862

Removing RewindBlockIndex() in bitcoin#21009 allows the following:

removal of tests using segwitheight=-1 in p2p_segwit.py.
move test_upgrade_after_activation() out of p2p_segwit.py reducing runtime
in turn, that allows us to drop support for -segwitheight=-1, which is only supported for that test.
that allows us to always set NODE_WITNESS in our local services. The only reason we don't do that is to support -segwitheight=-1.
that in turn allows us to drop all of the GetLocalServices() & NODE_WITNESS checks inside net_processing.cpp, since our local services would always include NODE_WITNESS

ACKs for top commit:
mzumsande:
Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647

Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

Please remove the italicized help prompts before submitting or merging

Provide a general summary of your changes in the Title above

Pull requests without a rationale and clear improvement may be closed
immediately.

Please provide clear motivation for your patch and explain how it improves
Dash Core user experience or Dash Core developer experience
significantly:

Any test improvements or new tests that improve coverage are always welcome.
All other changes should have accompanying unit tests (see src/test/) or
functional tests (see test/). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Dash Core, if possible.

Issue being fixed or feature implemented

Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.

What was done?

Describe your changes in detail

How Has This Been Tested?

Please describe in detail how you tested your changes.

Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.

Breaking Changes

Please describe any breaking changes your code introduces

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta) Pull request description: Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862 Removing `RewindBlockIndex()` in bitcoin#21009 allows the following: - removal of tests using `segwitheight=-1` in `p2p_segwit.py`. - move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime - in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test. - that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`. - that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS` ACKs for top commit: mzumsande: Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647 Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

…User-Space, Statically Defined Tracing (USDT) 8f37f5c tracing: Tracepoint for connected blocks (0xb10c) 4224dec tracing: Tracepoints for in- and outbound P2P msgs (0xb10c) 469b71a doc: document systemtap dependency (0xb10c) 84ace9a doc: Add initial USDT documentation (0xb10c) Pull request description: This PR adds documentation for User-Space, Statically Defined Tracing (USDT) as well as three tracepoints (including documentation and usage examples). ## Context The `TRACEx` macros for tracepoints and build system changes for USDT were merged in bitcoin#19866 earlier this year. Issue bitcoin#20981 contains discussion about potential tracepoints and guidelines for adding them (also documented with this PR). USDT was a topic in a [core-dev-meeting discussion](https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2021/bitcoin-core-dev.2021-01-21-19.00.moin.txt) on 21st Jan, 2021. - [collabora.com: An eBPF overview, part 1: Introduction](https://www.collabora.com/news-and-blog/blog/2019/04/05/an-ebpf-overview-part-1-introduction/) - [collabora.com: An eBPF overview, part 2: Machine & bytecode](https://www.collabora.com/news-and-blog/blog/2019/04/15/an-ebpf-overview-part-2-machine-and-bytecode/) - [Brendan D. Gregg's blog posts, and book on on eBPF](http://www.brendangregg.com/) - [Brendan D. Gregg: Linux bcc/BPF Node.js USDT Tracing](http://www.brendangregg.com/blog/2016-10-12/linux-bcc-nodejs-usdt.html) ## USDT? Stablecoin? User-space, Statically Defined Tracing (USDT) allows for more observability during development, debugging, code review, and production usage. The tracepoints make it possible to keep track of custom statistics and enable detailed monitoring of otherwise hidden internals and have little to no performance impact when unused. Linux kernels (4.x or newer) can hook into the tracepoints and execute [eBPF] programs in a kernel VM once the tracepoint is called. This PR includes, for example, tracepoints for in- and outbound P2P messages. ``` USDT and eBPF Overview ====================== ┌──────────────────┐ ┌──────────────┐ │ tracing script │ │ bitcoind │ │==================│ 2. │==============│ │ eBPF │ tracing │ hooks │ │ │ code │ logic │ into┌─┤►tracepoint 1─┼───┐ 3. └────┬───┴──▲──────┘ ├─┤►tracepoint 2 │ │ pass args 1. │ │ 4. │ │ ... │ │ to eBPF User compiles │ │ pass data to │ └──────────────┘ │ program Space & loads │ │ tracing script │ │ ─────────────────┼──────┼─────────────────┼────────────────────┼─── Kernel │ │ │ │ Space ┌──┬─▼──────┴─────────────────┴────────────┐ │ │ │ eBPF program │◄──────┘ │ └───────────────────────────────────────┤ │ eBPF kernel Virtual Machine (sandboxed) │ └──────────────────────────────────────────┘ 1. The tracing script compiles the eBPF code and loads the eBFP program into a kernel VM 2. The eBPF program hooks into one or more tracepoints 3. When the tracepoint is called, the arguments are passed to the eBPF program 4. The eBPF program processes the arguments and returns data to the tracing script ``` The two main [eBPF] front-ends with support for USDT are [bpftrace] an [BPF Compiler Collection (BCC)]. BCC is used for complex tools and daemons and `bpftrace` is preferred for one-liners and shorter scripts. Example tracing scripts for both are provided with this PR. [eBPF]: https://ebpf.io/ [bpftrace]: https://github.com/iovisor/bpftrace [BPF Compiler Collection (BCC)]: https://github.com/iovisor/bcc This PR adds three tracepoints: - `net:inbound_message` - `net:outbound_message` - `valildation:block_connected` See `doc/tracing.md` and `contrib/tracing/` for documentation and example tracing scripts. ## Open Questions (Not in scope for this PR) - How to use these tracepoints under macOS? - Release builds with USDT support? - Should and can the tracepoints be automatically tested? ## Todo (before undraft) - [x] bcc example showing how to read raw P2P messages up to 32kb - [x] document that you need `sys/sdt.h` from `systemtap` for USDT support in Bitcoin Core (`apt install systemtap-sdt-dev` on debian-like). See bitcoin@933ab8a - [ ] release notes? ACKs for top commit: laanwj: re-ACK 8f37f5c jb55: ACK 8f37f5c Tree-SHA512: a92a8a2dfcd28465f58a6e5f50d39486817ef5f51214ec40bdb02a6843b9c08ea154fadb31558825ff3a4687477b90f2a5da5d6451989eef978e128a264c289d

fadfd84 test: Remove unused connect_nodes_bi (MarcoFalke) fa3b9ee scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke) faaee1e test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke) 1111bb9 test: Reformat python imports to aid scripted diff (MarcoFalke) Pull request description: By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side). This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times. Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests. Historic background: `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug dashpay#5113 and dashpay#5138, which has long been fixed in dashpay#5157 and dashpay#5662. ACKs for top commit: laanwj: ACK fadfd84 jonasschnelli: utACK fadfd84 - more of less a cleanup PR. promag: Tested ACK fadfd84, ran extended tests. Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24

github-actions · 2024-01-28T04:57:47Z

This pull request has conflicts, please rebase.

vijaydasmp · 2023-11-01T17:06:02Z

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using -segwitheight=-1 (Dhruv Mehta)

Pull request description:

Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862

Removing RewindBlockIndex() in bitcoin#21009 allows the following:

removal of tests using segwitheight=-1 in p2p_segwit.py.
move test_upgrade_after_activation() out of p2p_segwit.py reducing runtime
in turn, that allows us to drop support for -segwitheight=-1, which is only supported for that test.
that allows us to always set NODE_WITNESS in our local services. The only reason we don't do that is to support -segwitheight=-1.
that in turn allows us to drop all of the GetLocalServices() & NODE_WITNESS checks inside net_processing.cpp, since our local services would always include NODE_WITNESS

ACKs for top commit:
mzumsande:
Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647

Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

Please remove the italicized help prompts before submitting or merging

Provide a general summary of your changes in the Title above

Pull requests without a rationale and clear improvement may be closed
immediately.

Please provide clear motivation for your patch and explain how it improves
Dash Core user experience or Dash Core developer experience
significantly:

Any test improvements or new tests that improve coverage are always welcome.
All other changes should have accompanying unit tests (see src/test/) or
functional tests (see test/). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Dash Core, if possible.

Issue being fixed or feature implemented

Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.

What was done?

Describe your changes in detail

How Has This Been Tested?

Please describe in detail how you tested your changes.

Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.

Breaking Changes

Please describe any breaking changes your code introduces

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta) Pull request description: Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862 Removing `RewindBlockIndex()` in bitcoin#21009 allows the following: - removal of tests using `segwitheight=-1` in `p2p_segwit.py`. - move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime - in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test. - that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`. - that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS` ACKs for top commit: mzumsande: Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647 Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

…User-Space, Statically Defined Tracing (USDT) 8f37f5c tracing: Tracepoint for connected blocks (0xb10c) 4224dec tracing: Tracepoints for in- and outbound P2P msgs (0xb10c) 469b71a doc: document systemtap dependency (0xb10c) 84ace9a doc: Add initial USDT documentation (0xb10c) Pull request description: This PR adds documentation for User-Space, Statically Defined Tracing (USDT) as well as three tracepoints (including documentation and usage examples). ## Context The `TRACEx` macros for tracepoints and build system changes for USDT were merged in bitcoin#19866 earlier this year. Issue bitcoin#20981 contains discussion about potential tracepoints and guidelines for adding them (also documented with this PR). USDT was a topic in a [core-dev-meeting discussion](https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2021/bitcoin-core-dev.2021-01-21-19.00.moin.txt) on 21st Jan, 2021. - [collabora.com: An eBPF overview, part 1: Introduction](https://www.collabora.com/news-and-blog/blog/2019/04/05/an-ebpf-overview-part-1-introduction/) - [collabora.com: An eBPF overview, part 2: Machine & bytecode](https://www.collabora.com/news-and-blog/blog/2019/04/15/an-ebpf-overview-part-2-machine-and-bytecode/) - [Brendan D. Gregg's blog posts, and book on on eBPF](http://www.brendangregg.com/) - [Brendan D. Gregg: Linux bcc/BPF Node.js USDT Tracing](http://www.brendangregg.com/blog/2016-10-12/linux-bcc-nodejs-usdt.html) ## USDT? Stablecoin? User-space, Statically Defined Tracing (USDT) allows for more observability during development, debugging, code review, and production usage. The tracepoints make it possible to keep track of custom statistics and enable detailed monitoring of otherwise hidden internals and have little to no performance impact when unused. Linux kernels (4.x or newer) can hook into the tracepoints and execute [eBPF] programs in a kernel VM once the tracepoint is called. This PR includes, for example, tracepoints for in- and outbound P2P messages. ``` USDT and eBPF Overview ====================== ┌──────────────────┐ ┌──────────────┐ │ tracing script │ │ bitcoind │ │==================│ 2. │==============│ │ eBPF │ tracing │ hooks │ │ │ code │ logic │ into┌─┤►tracepoint 1─┼───┐ 3. └────┬───┴──▲──────┘ ├─┤►tracepoint 2 │ │ pass args 1. │ │ 4. │ │ ... │ │ to eBPF User compiles │ │ pass data to │ └──────────────┘ │ program Space & loads │ │ tracing script │ │ ─────────────────┼──────┼─────────────────┼────────────────────┼─── Kernel │ │ │ │ Space ┌──┬─▼──────┴─────────────────┴────────────┐ │ │ │ eBPF program │◄──────┘ │ └───────────────────────────────────────┤ │ eBPF kernel Virtual Machine (sandboxed) │ └──────────────────────────────────────────┘ 1. The tracing script compiles the eBPF code and loads the eBFP program into a kernel VM 2. The eBPF program hooks into one or more tracepoints 3. When the tracepoint is called, the arguments are passed to the eBPF program 4. The eBPF program processes the arguments and returns data to the tracing script ``` The two main [eBPF] front-ends with support for USDT are [bpftrace] an [BPF Compiler Collection (BCC)]. BCC is used for complex tools and daemons and `bpftrace` is preferred for one-liners and shorter scripts. Example tracing scripts for both are provided with this PR. [eBPF]: https://ebpf.io/ [bpftrace]: https://github.com/iovisor/bpftrace [BPF Compiler Collection (BCC)]: https://github.com/iovisor/bcc This PR adds three tracepoints: - `net:inbound_message` - `net:outbound_message` - `valildation:block_connected` See `doc/tracing.md` and `contrib/tracing/` for documentation and example tracing scripts. ## Open Questions (Not in scope for this PR) - How to use these tracepoints under macOS? - Release builds with USDT support? - Should and can the tracepoints be automatically tested? ## Todo (before undraft) - [x] bcc example showing how to read raw P2P messages up to 32kb - [x] document that you need `sys/sdt.h` from `systemtap` for USDT support in Bitcoin Core (`apt install systemtap-sdt-dev` on debian-like). See bitcoin@933ab8a - [ ] release notes? ACKs for top commit: laanwj: re-ACK 8f37f5c jb55: ACK 8f37f5c Tree-SHA512: a92a8a2dfcd28465f58a6e5f50d39486817ef5f51214ec40bdb02a6843b9c08ea154fadb31558825ff3a4687477b90f2a5da5d6451989eef978e128a264c289d

fadfd84 test: Remove unused connect_nodes_bi (MarcoFalke) fa3b9ee scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke) faaee1e test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke) 1111bb9 test: Reformat python imports to aid scripted diff (MarcoFalke) Pull request description: By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side). This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times. Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests. Historic background: `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug dashpay#5113 and dashpay#5138, which has long been fixed in dashpay#5157 and dashpay#5662. ACKs for top commit: laanwj: ACK fadfd84 jonasschnelli: utACK fadfd84 - more of less a cleanup PR. promag: Tested ACK fadfd84, ran extended tests. Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24

github-actions · 2024-01-28T04:57:47Z

This pull request has conflicts, please rebase.

vijaydasmp · 2023-11-01T17:06:02Z

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using -segwitheight=-1 (Dhruv Mehta)

Pull request description:

Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862

Removing RewindBlockIndex() in bitcoin#21009 allows the following:

removal of tests using segwitheight=-1 in p2p_segwit.py.
move test_upgrade_after_activation() out of p2p_segwit.py reducing runtime
in turn, that allows us to drop support for -segwitheight=-1, which is only supported for that test.
that allows us to always set NODE_WITNESS in our local services. The only reason we don't do that is to support -segwitheight=-1.
that in turn allows us to drop all of the GetLocalServices() & NODE_WITNESS checks inside net_processing.cpp, since our local services would always include NODE_WITNESS

ACKs for top commit:
mzumsande:
Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647

Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

Please remove the italicized help prompts before submitting or merging

Provide a general summary of your changes in the Title above

Pull requests without a rationale and clear improvement may be closed
immediately.

Please provide clear motivation for your patch and explain how it improves
Dash Core user experience or Dash Core developer experience
significantly:

Any test improvements or new tests that improve coverage are always welcome.
All other changes should have accompanying unit tests (see src/test/) or
functional tests (see test/). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Dash Core, if possible.

Issue being fixed or feature implemented

Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.

What was done?

Describe your changes in detail

How Has This Been Tested?

Please describe in detail how you tested your changes.

Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.

Breaking Changes

Please describe any breaking changes your code introduces

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta) Pull request description: Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862 Removing `RewindBlockIndex()` in bitcoin#21009 allows the following: - removal of tests using `segwitheight=-1` in `p2p_segwit.py`. - move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime - in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test. - that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`. - that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS` ACKs for top commit: mzumsande: Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647 Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

…User-Space, Statically Defined Tracing (USDT) 8f37f5c tracing: Tracepoint for connected blocks (0xb10c) 4224dec tracing: Tracepoints for in- and outbound P2P msgs (0xb10c) 469b71a doc: document systemtap dependency (0xb10c) 84ace9a doc: Add initial USDT documentation (0xb10c) Pull request description: This PR adds documentation for User-Space, Statically Defined Tracing (USDT) as well as three tracepoints (including documentation and usage examples). ## Context The `TRACEx` macros for tracepoints and build system changes for USDT were merged in bitcoin#19866 earlier this year. Issue bitcoin#20981 contains discussion about potential tracepoints and guidelines for adding them (also documented with this PR). USDT was a topic in a [core-dev-meeting discussion](https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2021/bitcoin-core-dev.2021-01-21-19.00.moin.txt) on 21st Jan, 2021. - [collabora.com: An eBPF overview, part 1: Introduction](https://www.collabora.com/news-and-blog/blog/2019/04/05/an-ebpf-overview-part-1-introduction/) - [collabora.com: An eBPF overview, part 2: Machine & bytecode](https://www.collabora.com/news-and-blog/blog/2019/04/15/an-ebpf-overview-part-2-machine-and-bytecode/) - [Brendan D. Gregg's blog posts, and book on on eBPF](http://www.brendangregg.com/) - [Brendan D. Gregg: Linux bcc/BPF Node.js USDT Tracing](http://www.brendangregg.com/blog/2016-10-12/linux-bcc-nodejs-usdt.html) ## USDT? Stablecoin? User-space, Statically Defined Tracing (USDT) allows for more observability during development, debugging, code review, and production usage. The tracepoints make it possible to keep track of custom statistics and enable detailed monitoring of otherwise hidden internals and have little to no performance impact when unused. Linux kernels (4.x or newer) can hook into the tracepoints and execute [eBPF] programs in a kernel VM once the tracepoint is called. This PR includes, for example, tracepoints for in- and outbound P2P messages. ``` USDT and eBPF Overview ====================== ┌──────────────────┐ ┌──────────────┐ │ tracing script │ │ bitcoind │ │==================│ 2. │==============│ │ eBPF │ tracing │ hooks │ │ │ code │ logic │ into┌─┤►tracepoint 1─┼───┐ 3. └────┬───┴──▲──────┘ ├─┤►tracepoint 2 │ │ pass args 1. │ │ 4. │ │ ... │ │ to eBPF User compiles │ │ pass data to │ └──────────────┘ │ program Space & loads │ │ tracing script │ │ ─────────────────┼──────┼─────────────────┼────────────────────┼─── Kernel │ │ │ │ Space ┌──┬─▼──────┴─────────────────┴────────────┐ │ │ │ eBPF program │◄──────┘ │ └───────────────────────────────────────┤ │ eBPF kernel Virtual Machine (sandboxed) │ └──────────────────────────────────────────┘ 1. The tracing script compiles the eBPF code and loads the eBFP program into a kernel VM 2. The eBPF program hooks into one or more tracepoints 3. When the tracepoint is called, the arguments are passed to the eBPF program 4. The eBPF program processes the arguments and returns data to the tracing script ``` The two main [eBPF] front-ends with support for USDT are [bpftrace] an [BPF Compiler Collection (BCC)]. BCC is used for complex tools and daemons and `bpftrace` is preferred for one-liners and shorter scripts. Example tracing scripts for both are provided with this PR. [eBPF]: https://ebpf.io/ [bpftrace]: https://github.com/iovisor/bpftrace [BPF Compiler Collection (BCC)]: https://github.com/iovisor/bcc This PR adds three tracepoints: - `net:inbound_message` - `net:outbound_message` - `valildation:block_connected` See `doc/tracing.md` and `contrib/tracing/` for documentation and example tracing scripts. ## Open Questions (Not in scope for this PR) - How to use these tracepoints under macOS? - Release builds with USDT support? - Should and can the tracepoints be automatically tested? ## Todo (before undraft) - [x] bcc example showing how to read raw P2P messages up to 32kb - [x] document that you need `sys/sdt.h` from `systemtap` for USDT support in Bitcoin Core (`apt install systemtap-sdt-dev` on debian-like). See bitcoin@933ab8a - [ ] release notes? ACKs for top commit: laanwj: re-ACK 8f37f5c jb55: ACK 8f37f5c Tree-SHA512: a92a8a2dfcd28465f58a6e5f50d39486817ef5f51214ec40bdb02a6843b9c08ea154fadb31558825ff3a4687477b90f2a5da5d6451989eef978e128a264c289d

fadfd84 test: Remove unused connect_nodes_bi (MarcoFalke) fa3b9ee scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke) faaee1e test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke) 1111bb9 test: Reformat python imports to aid scripted diff (MarcoFalke) Pull request description: By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side). This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times. Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests. Historic background: `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug dashpay#5113 and dashpay#5138, which has long been fixed in dashpay#5157 and dashpay#5662. ACKs for top commit: laanwj: ACK fadfd84 jonasschnelli: utACK fadfd84 - more of less a cleanup PR. promag: Tested ACK fadfd84, ran extended tests. Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24

github-actions · 2024-01-28T04:57:47Z

This pull request has conflicts, please rebase.

vijaydasmp · 2023-11-01T17:06:02Z

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using -segwitheight=-1 (Dhruv Mehta)

Pull request description:

Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862

Removing RewindBlockIndex() in bitcoin#21009 allows the following:

removal of tests using segwitheight=-1 in p2p_segwit.py.
move test_upgrade_after_activation() out of p2p_segwit.py reducing runtime
in turn, that allows us to drop support for -segwitheight=-1, which is only supported for that test.
that allows us to always set NODE_WITNESS in our local services. The only reason we don't do that is to support -segwitheight=-1.
that in turn allows us to drop all of the GetLocalServices() & NODE_WITNESS checks inside net_processing.cpp, since our local services would always include NODE_WITNESS

ACKs for top commit:
mzumsande:
Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647

Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

Please remove the italicized help prompts before submitting or merging

Provide a general summary of your changes in the Title above

Pull requests without a rationale and clear improvement may be closed
immediately.

Please provide clear motivation for your patch and explain how it improves
Dash Core user experience or Dash Core developer experience
significantly:

Any test improvements or new tests that improve coverage are always welcome.
All other changes should have accompanying unit tests (see src/test/) or
functional tests (see test/). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Dash Core, if possible.

Issue being fixed or feature implemented

Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.

What was done?

Describe your changes in detail

How Has This Been Tested?

Please describe in detail how you tested your changes.

Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.

Breaking Changes

Please describe any breaking changes your code introduces

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta) Pull request description: Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862 Removing `RewindBlockIndex()` in bitcoin#21009 allows the following: - removal of tests using `segwitheight=-1` in `p2p_segwit.py`. - move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime - in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test. - that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`. - that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS` ACKs for top commit: mzumsande: Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647 Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

…User-Space, Statically Defined Tracing (USDT) 8f37f5c tracing: Tracepoint for connected blocks (0xb10c) 4224dec tracing: Tracepoints for in- and outbound P2P msgs (0xb10c) 469b71a doc: document systemtap dependency (0xb10c) 84ace9a doc: Add initial USDT documentation (0xb10c) Pull request description: This PR adds documentation for User-Space, Statically Defined Tracing (USDT) as well as three tracepoints (including documentation and usage examples). ## Context The `TRACEx` macros for tracepoints and build system changes for USDT were merged in bitcoin#19866 earlier this year. Issue bitcoin#20981 contains discussion about potential tracepoints and guidelines for adding them (also documented with this PR). USDT was a topic in a [core-dev-meeting discussion](https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2021/bitcoin-core-dev.2021-01-21-19.00.moin.txt) on 21st Jan, 2021. - [collabora.com: An eBPF overview, part 1: Introduction](https://www.collabora.com/news-and-blog/blog/2019/04/05/an-ebpf-overview-part-1-introduction/) - [collabora.com: An eBPF overview, part 2: Machine & bytecode](https://www.collabora.com/news-and-blog/blog/2019/04/15/an-ebpf-overview-part-2-machine-and-bytecode/) - [Brendan D. Gregg's blog posts, and book on on eBPF](http://www.brendangregg.com/) - [Brendan D. Gregg: Linux bcc/BPF Node.js USDT Tracing](http://www.brendangregg.com/blog/2016-10-12/linux-bcc-nodejs-usdt.html) ## USDT? Stablecoin? User-space, Statically Defined Tracing (USDT) allows for more observability during development, debugging, code review, and production usage. The tracepoints make it possible to keep track of custom statistics and enable detailed monitoring of otherwise hidden internals and have little to no performance impact when unused. Linux kernels (4.x or newer) can hook into the tracepoints and execute [eBPF] programs in a kernel VM once the tracepoint is called. This PR includes, for example, tracepoints for in- and outbound P2P messages. ``` USDT and eBPF Overview ====================== ┌──────────────────┐ ┌──────────────┐ │ tracing script │ │ bitcoind │ │==================│ 2. │==============│ │ eBPF │ tracing │ hooks │ │ │ code │ logic │ into┌─┤►tracepoint 1─┼───┐ 3. └────┬───┴──▲──────┘ ├─┤►tracepoint 2 │ │ pass args 1. │ │ 4. │ │ ... │ │ to eBPF User compiles │ │ pass data to │ └──────────────┘ │ program Space & loads │ │ tracing script │ │ ─────────────────┼──────┼─────────────────┼────────────────────┼─── Kernel │ │ │ │ Space ┌──┬─▼──────┴─────────────────┴────────────┐ │ │ │ eBPF program │◄──────┘ │ └───────────────────────────────────────┤ │ eBPF kernel Virtual Machine (sandboxed) │ └──────────────────────────────────────────┘ 1. The tracing script compiles the eBPF code and loads the eBFP program into a kernel VM 2. The eBPF program hooks into one or more tracepoints 3. When the tracepoint is called, the arguments are passed to the eBPF program 4. The eBPF program processes the arguments and returns data to the tracing script ``` The two main [eBPF] front-ends with support for USDT are [bpftrace] an [BPF Compiler Collection (BCC)]. BCC is used for complex tools and daemons and `bpftrace` is preferred for one-liners and shorter scripts. Example tracing scripts for both are provided with this PR. [eBPF]: https://ebpf.io/ [bpftrace]: https://github.com/iovisor/bpftrace [BPF Compiler Collection (BCC)]: https://github.com/iovisor/bcc This PR adds three tracepoints: - `net:inbound_message` - `net:outbound_message` - `valildation:block_connected` See `doc/tracing.md` and `contrib/tracing/` for documentation and example tracing scripts. ## Open Questions (Not in scope for this PR) - How to use these tracepoints under macOS? - Release builds with USDT support? - Should and can the tracepoints be automatically tested? ## Todo (before undraft) - [x] bcc example showing how to read raw P2P messages up to 32kb - [x] document that you need `sys/sdt.h` from `systemtap` for USDT support in Bitcoin Core (`apt install systemtap-sdt-dev` on debian-like). See bitcoin@933ab8a - [ ] release notes? ACKs for top commit: laanwj: re-ACK 8f37f5c jb55: ACK 8f37f5c Tree-SHA512: a92a8a2dfcd28465f58a6e5f50d39486817ef5f51214ec40bdb02a6843b9c08ea154fadb31558825ff3a4687477b90f2a5da5d6451989eef978e128a264c289d

fadfd84 test: Remove unused connect_nodes_bi (MarcoFalke) fa3b9ee scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke) faaee1e test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke) 1111bb9 test: Reformat python imports to aid scripted diff (MarcoFalke) Pull request description: By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side). This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times. Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests. Historic background: `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug dashpay#5113 and dashpay#5138, which has long been fixed in dashpay#5157 and dashpay#5662. ACKs for top commit: laanwj: ACK fadfd84 jonasschnelli: utACK fadfd84 - more of less a cleanup PR. promag: Tested ACK fadfd84, ran extended tests. Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24

github-actions · 2024-01-28T04:57:47Z

This pull request has conflicts, please rebase.

vijaydasmp · 2023-11-01T17:06:02Z

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using -segwitheight=-1 (Dhruv Mehta)

Pull request description:

Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862

Removing RewindBlockIndex() in bitcoin#21009 allows the following:

removal of tests using segwitheight=-1 in p2p_segwit.py.
move test_upgrade_after_activation() out of p2p_segwit.py reducing runtime
in turn, that allows us to drop support for -segwitheight=-1, which is only supported for that test.
that allows us to always set NODE_WITNESS in our local services. The only reason we don't do that is to support -segwitheight=-1.
that in turn allows us to drop all of the GetLocalServices() & NODE_WITNESS checks inside net_processing.cpp, since our local services would always include NODE_WITNESS

ACKs for top commit:
mzumsande:
Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647

Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

Please remove the italicized help prompts before submitting or merging

Provide a general summary of your changes in the Title above

Pull requests without a rationale and clear improvement may be closed
immediately.

Please provide clear motivation for your patch and explain how it improves
Dash Core user experience or Dash Core developer experience
significantly:

Any test improvements or new tests that improve coverage are always welcome.
All other changes should have accompanying unit tests (see src/test/) or
functional tests (see test/). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Dash Core, if possible.

Issue being fixed or feature implemented

Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.

What was done?

Describe your changes in detail

How Has This Been Tested?

Please describe in detail how you tested your changes.

Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.

Breaking Changes

Please describe any breaking changes your code introduces

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

a806647 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta) 189128c [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta) ac82b99 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta) 6f8b198 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta) eba5b1c [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta) Pull request description: Builds on bitcoin#21009 and makes progress on remaining items in bitcoin#17862 Removing `RewindBlockIndex()` in bitcoin#21009 allows the following: - removal of tests using `segwitheight=-1` in `p2p_segwit.py`. - move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime - in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test. - that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`. - that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS` ACKs for top commit: mzumsande: Code-Review ACK a806647 laanwj: Code review ACK a806647, nice cleanup jnewbery: utACK a806647 theStack: ACK a806647 Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

…User-Space, Statically Defined Tracing (USDT) 8f37f5c tracing: Tracepoint for connected blocks (0xb10c) 4224dec tracing: Tracepoints for in- and outbound P2P msgs (0xb10c) 469b71a doc: document systemtap dependency (0xb10c) 84ace9a doc: Add initial USDT documentation (0xb10c) Pull request description: This PR adds documentation for User-Space, Statically Defined Tracing (USDT) as well as three tracepoints (including documentation and usage examples). ## Context The `TRACEx` macros for tracepoints and build system changes for USDT were merged in bitcoin#19866 earlier this year. Issue bitcoin#20981 contains discussion about potential tracepoints and guidelines for adding them (also documented with this PR). USDT was a topic in a [core-dev-meeting discussion](https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2021/bitcoin-core-dev.2021-01-21-19.00.moin.txt) on 21st Jan, 2021. - [collabora.com: An eBPF overview, part 1: Introduction](https://www.collabora.com/news-and-blog/blog/2019/04/05/an-ebpf-overview-part-1-introduction/) - [collabora.com: An eBPF overview, part 2: Machine & bytecode](https://www.collabora.com/news-and-blog/blog/2019/04/15/an-ebpf-overview-part-2-machine-and-bytecode/) - [Brendan D. Gregg's blog posts, and book on on eBPF](http://www.brendangregg.com/) - [Brendan D. Gregg: Linux bcc/BPF Node.js USDT Tracing](http://www.brendangregg.com/blog/2016-10-12/linux-bcc-nodejs-usdt.html) ## USDT? Stablecoin? User-space, Statically Defined Tracing (USDT) allows for more observability during development, debugging, code review, and production usage. The tracepoints make it possible to keep track of custom statistics and enable detailed monitoring of otherwise hidden internals and have little to no performance impact when unused. Linux kernels (4.x or newer) can hook into the tracepoints and execute [eBPF] programs in a kernel VM once the tracepoint is called. This PR includes, for example, tracepoints for in- and outbound P2P messages. ``` USDT and eBPF Overview ====================== ┌──────────────────┐ ┌──────────────┐ │ tracing script │ │ bitcoind │ │==================│ 2. │==============│ │ eBPF │ tracing │ hooks │ │ │ code │ logic │ into┌─┤►tracepoint 1─┼───┐ 3. └────┬───┴──▲──────┘ ├─┤►tracepoint 2 │ │ pass args 1. │ │ 4. │ │ ... │ │ to eBPF User compiles │ │ pass data to │ └──────────────┘ │ program Space & loads │ │ tracing script │ │ ─────────────────┼──────┼─────────────────┼────────────────────┼─── Kernel │ │ │ │ Space ┌──┬─▼──────┴─────────────────┴────────────┐ │ │ │ eBPF program │◄──────┘ │ └───────────────────────────────────────┤ │ eBPF kernel Virtual Machine (sandboxed) │ └──────────────────────────────────────────┘ 1. The tracing script compiles the eBPF code and loads the eBFP program into a kernel VM 2. The eBPF program hooks into one or more tracepoints 3. When the tracepoint is called, the arguments are passed to the eBPF program 4. The eBPF program processes the arguments and returns data to the tracing script ``` The two main [eBPF] front-ends with support for USDT are [bpftrace] an [BPF Compiler Collection (BCC)]. BCC is used for complex tools and daemons and `bpftrace` is preferred for one-liners and shorter scripts. Example tracing scripts for both are provided with this PR. [eBPF]: https://ebpf.io/ [bpftrace]: https://github.com/iovisor/bpftrace [BPF Compiler Collection (BCC)]: https://github.com/iovisor/bcc This PR adds three tracepoints: - `net:inbound_message` - `net:outbound_message` - `valildation:block_connected` See `doc/tracing.md` and `contrib/tracing/` for documentation and example tracing scripts. ## Open Questions (Not in scope for this PR) - How to use these tracepoints under macOS? - Release builds with USDT support? - Should and can the tracepoints be automatically tested? ## Todo (before undraft) - [x] bcc example showing how to read raw P2P messages up to 32kb - [x] document that you need `sys/sdt.h` from `systemtap` for USDT support in Bitcoin Core (`apt install systemtap-sdt-dev` on debian-like). See bitcoin@933ab8a - [ ] release notes? ACKs for top commit: laanwj: re-ACK 8f37f5c jb55: ACK 8f37f5c Tree-SHA512: a92a8a2dfcd28465f58a6e5f50d39486817ef5f51214ec40bdb02a6843b9c08ea154fadb31558825ff3a4687477b90f2a5da5d6451989eef978e128a264c289d

fadfd84 test: Remove unused connect_nodes_bi (MarcoFalke) fa3b9ee scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke) faaee1e test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke) 1111bb9 test: Reformat python imports to aid scripted diff (MarcoFalke) Pull request description: By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side). This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times. Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests. Historic background: `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug dashpay#5113 and dashpay#5138, which has long been fixed in dashpay#5157 and dashpay#5662. ACKs for top commit: laanwj: ACK fadfd84 jonasschnelli: utACK fadfd84 - more of less a cleanup PR. promag: Tested ACK fadfd84, ran extended tests. Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24

github-actions · 2024-01-28T04:57:47Z

This pull request has conflicts, please rebase.

laanwj added 2 commits November 1, 2023 22:35

vijaydasmp force-pushed the bp23_1 branch from 54bcffb to c45e0c6Compare November 6, 2023 18:05

vijaydasmp changed the title ~~Merge bitcoin/bitcoin#21090: Default to NODE_WITNESS in nLocalServices~~backport: Merge bitcoin#21090Nov 6, 2023

vijaydasmp changed the title ~~backport: Merge bitcoin#21090~~backport: Merge bitcoin#21090,22006Nov 6, 2023

github-actionsBot added the needs rebase label Jan 28, 2024

vijaydasmp closed this Feb 19, 2024

vijaydasmp deleted the bp23_1 branch February 19, 2024 15:37

laanwj added 2 commits November 1, 2023 22:35

vijaydasmp force-pushed the bp23_1 branch from 54bcffb to c45e0c6Compare November 6, 2023 18:05

vijaydasmp changed the title ~~Merge bitcoin/bitcoin#21090: Default to NODE_WITNESS in nLocalServices~~backport: Merge bitcoin#21090Nov 6, 2023

vijaydasmp changed the title ~~backport: Merge bitcoin#21090~~backport: Merge bitcoin#21090,22006Nov 6, 2023

github-actionsBot added the needs rebase label Jan 28, 2024

vijaydasmp closed this Feb 19, 2024

vijaydasmp deleted the bp23_1 branch February 19, 2024 15:37

laanwj added 2 commits November 1, 2023 22:35

vijaydasmp force-pushed the bp23_1 branch from 54bcffb to c45e0c6Compare November 6, 2023 18:05

vijaydasmp changed the title ~~Merge bitcoin/bitcoin#21090: Default to NODE_WITNESS in nLocalServices~~backport: Merge bitcoin#21090Nov 6, 2023

vijaydasmp changed the title ~~backport: Merge bitcoin#21090~~backport: Merge bitcoin#21090,22006Nov 6, 2023

github-actionsBot added the needs rebase label Jan 28, 2024

vijaydasmp closed this Feb 19, 2024

vijaydasmp deleted the bp23_1 branch February 19, 2024 15:37

laanwj added 2 commits November 1, 2023 22:35

vijaydasmp force-pushed the bp23_1 branch from 54bcffb to c45e0c6Compare November 6, 2023 18:05

vijaydasmp changed the title ~~Merge bitcoin/bitcoin#21090: Default to NODE_WITNESS in nLocalServices~~backport: Merge bitcoin#21090Nov 6, 2023

vijaydasmp changed the title ~~backport: Merge bitcoin#21090~~backport: Merge bitcoin#21090,22006Nov 6, 2023

github-actionsBot added the needs rebase label Jan 28, 2024

vijaydasmp closed this Feb 19, 2024

vijaydasmp deleted the bp23_1 branch February 19, 2024 15:37

laanwj added 2 commits November 1, 2023 22:35

vijaydasmp force-pushed the bp23_1 branch from 54bcffb to c45e0c6Compare November 6, 2023 18:05

vijaydasmp changed the title ~~Merge bitcoin/bitcoin#21090: Default to NODE_WITNESS in nLocalServices~~backport: Merge bitcoin#21090Nov 6, 2023

vijaydasmp changed the title ~~backport: Merge bitcoin#21090~~backport: Merge bitcoin#21090,22006Nov 6, 2023

github-actionsBot added the needs rebase label Jan 28, 2024

vijaydasmp closed this Feb 19, 2024

vijaydasmp deleted the bp23_1 branch February 19, 2024 15:37

laanwj added 2 commits November 1, 2023 22:35

vijaydasmp force-pushed the bp23_1 branch from 54bcffb to c45e0c6Compare November 6, 2023 18:05

vijaydasmp changed the title ~~Merge bitcoin/bitcoin#21090: Default to NODE_WITNESS in nLocalServices~~backport: Merge bitcoin#21090Nov 6, 2023

vijaydasmp changed the title ~~backport: Merge bitcoin#21090~~backport: Merge bitcoin#21090,22006Nov 6, 2023

github-actionsBot added the needs rebase label Jan 28, 2024

vijaydasmp closed this Feb 19, 2024

vijaydasmp deleted the bp23_1 branch February 19, 2024 15:37

laanwj added 2 commits November 1, 2023 22:35

vijaydasmp force-pushed the bp23_1 branch from 54bcffb to c45e0c6Compare November 6, 2023 18:05

vijaydasmp changed the title ~~Merge bitcoin/bitcoin#21090: Default to NODE_WITNESS in nLocalServices~~backport: Merge bitcoin#21090Nov 6, 2023

vijaydasmp changed the title ~~backport: Merge bitcoin#21090~~backport: Merge bitcoin#21090,22006Nov 6, 2023

github-actionsBot added the needs rebase label Jan 28, 2024

vijaydasmp closed this Feb 19, 2024

vijaydasmp deleted the bp23_1 branch February 19, 2024 15:37

laanwj added 2 commits November 1, 2023 22:35

vijaydasmp force-pushed the bp23_1 branch from 54bcffb to c45e0c6Compare November 6, 2023 18:05

vijaydasmp changed the title ~~Merge bitcoin/bitcoin#21090: Default to NODE_WITNESS in nLocalServices~~backport: Merge bitcoin#21090Nov 6, 2023

vijaydasmp changed the title ~~backport: Merge bitcoin#21090~~backport: Merge bitcoin#21090,22006Nov 6, 2023

github-actionsBot added the needs rebase label Jan 28, 2024

vijaydasmp closed this Feb 19, 2024

vijaydasmp deleted the bp23_1 branch February 19, 2024 15:37

Uh oh!

Conversation

vijaydasmp commented Nov 1, 2023

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actionsBot commented Jan 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

vijaydasmp commented Nov 1, 2023

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actionsBot commented Jan 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

vijaydasmp commented Nov 1, 2023

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actionsBot commented Jan 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

vijaydasmp commented Nov 1, 2023

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actionsBot commented Jan 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

vijaydasmp commented Nov 1, 2023

Issue being fixed or feature implemented