Skip to content

Conversation

vijaydasmp
Copy link

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
@vijaydasmp vijaydasmp changed the title Merge bitcoin/bitcoin#21090: Default to NODE_WITNESS in nLocalServices backport: Merge bitcoin#21090 Nov 6, 2023
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#21090 backport: Merge bitcoin#21090,22006 Nov 6, 2023
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Nov 20, 2023
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
gades pushed a commit to piratecash/pirate that referenced this pull request Dec 9, 2023
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
Copy link

This pull request has conflicts, please rebase.

@vijaydasmp vijaydasmp closed this Feb 19, 2024
@vijaydasmp vijaydasmp deleted the bp23_1 branch February 19, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants