Skip to content

Conversation

ismaelsadeeq
Copy link
Member

@ismaelsadeeq ismaelsadeeq commented Jan 24, 2024

BroadcastTransaction is also called by submitpackage RPC.

All transactions that are accepted into the mempool post package processing are broadcasted to peers individually here

const auto err = BroadcastTransaction(node, tx, err_string, /*max_tx_fee=*/0, /*relay=*/true, /*wait_callback=*/true);

It's not maintainable to list all the callers of a function.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 24, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK stickies-v, kristapsk, naumenkogs
Stale ACK vasild, BrandonOdiwuor, willcl-ark, shaavan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Docs label Jan 24, 2024
@ismaelsadeeq ismaelsadeeq changed the title doc: clarify that BroadcastTransaction comment doc: clarify BroadcastTransaction comment Jan 24, 2024
@ismaelsadeeq ismaelsadeeq force-pushed the 01-2024-clarify-BroadcastTransaction-comment branch from 2272c3a to cca20d9 Compare January 24, 2024 15:38
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK cca20d9

I was looking at the same comment and was thinking it needs an update. Thanks!

Copy link
Contributor

@BrandonOdiwuor BrandonOdiwuor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm ACK cca20d9

Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK cca20d9

Copy link
Member

@willcl-ark willcl-ark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK cca20d9

@ismaelsadeeq ismaelsadeeq force-pushed the 01-2024-clarify-BroadcastTransaction-comment branch from cca20d9 to 437e8eb Compare January 26, 2024 09:40
@ismaelsadeeq ismaelsadeeq changed the title doc: clarify BroadcastTransaction comment doc: update BroadcastTransaction comment Jan 26, 2024
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 437e8eb

Following the discussion in the PR so far, I agree that it is more logical and maintainable not to detail which function calls (or can call) a particular function since this information is easily accessible through other methods (e.g. by using an IDE)
That's why I think it's best to remove the comment.

@DrahtBot DrahtBot requested review from willcl-ark, kristapsk, vasild and BrandonOdiwuor and removed request for kristapsk and BrandonOdiwuor January 26, 2024 12:51
BroadcastTransaction is also called by submitpackage RPC.

It's not maintainable to list all the callers of a function.
@ismaelsadeeq ismaelsadeeq force-pushed the 01-2024-clarify-BroadcastTransaction-comment branch from 437e8eb to 31cce4a Compare January 29, 2024 12:10
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 31cce4a

Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 31cce4a

@DrahtBot DrahtBot requested review from BrandonOdiwuor and shaavan and removed request for shaavan and BrandonOdiwuor January 29, 2024 13:09
Copy link
Member

@naumenkogs naumenkogs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 31cce4a

@DrahtBot DrahtBot requested review from BrandonOdiwuor and shaavan and removed request for BrandonOdiwuor and shaavan January 30, 2024 09:04
@glozow glozow merged commit cad2df2 into bitcoin:master Jan 30, 2024
@ismaelsadeeq ismaelsadeeq deleted the 01-2024-clarify-BroadcastTransaction-comment branch June 27, 2024 11:18
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
31cce4a doc: update `BroadcastTransaction` comment (ismaelsadeeq)

Pull request description:

  `BroadcastTransaction` is also called by `submitpackage` RPC.

  All transactions that are accepted into the mempool post package processing are broadcasted to peers individually here
  https://github.com/bitcoin/bitcoin/blob/ea4ddd8652d9dd1e7698e2a6f84c606cf24a2e3e/src/rpc/mempool.cpp#L926

  It's not maintainable to list all the callers of a function.

ACKs for top commit:
  stickies-v:
    ACK 31cce4a
  kristapsk:
    ACK 31cce4a
  naumenkogs:
    ACK 31cce4a

Tree-SHA512: 8aea92c53c1911a0ac36fe9e3a24d37d83e7d9b40a16f0832bfa7a719328697621e3f94a5dc80d1840e7ae705e0c3aab7a3df7064986e1e53a4a4114adf078a8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
31cce4a doc: update `BroadcastTransaction` comment (ismaelsadeeq)

Pull request description:

  `BroadcastTransaction` is also called by `submitpackage` RPC.

  All transactions that are accepted into the mempool post package processing are broadcasted to peers individually here
  https://github.com/bitcoin/bitcoin/blob/ea4ddd8652d9dd1e7698e2a6f84c606cf24a2e3e/src/rpc/mempool.cpp#L926

  It's not maintainable to list all the callers of a function.

ACKs for top commit:
  stickies-v:
    ACK 31cce4a
  kristapsk:
    ACK 31cce4a
  naumenkogs:
    ACK 31cce4a

Tree-SHA512: 8aea92c53c1911a0ac36fe9e3a24d37d83e7d9b40a16f0832bfa7a719328697621e3f94a5dc80d1840e7ae705e0c3aab7a3df7064986e1e53a4a4114adf078a8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
31cce4a doc: update `BroadcastTransaction` comment (ismaelsadeeq)

Pull request description:

  `BroadcastTransaction` is also called by `submitpackage` RPC.

  All transactions that are accepted into the mempool post package processing are broadcasted to peers individually here
  https://github.com/bitcoin/bitcoin/blob/ea4ddd8652d9dd1e7698e2a6f84c606cf24a2e3e/src/rpc/mempool.cpp#L926

  It's not maintainable to list all the callers of a function.

ACKs for top commit:
  stickies-v:
    ACK 31cce4a
  kristapsk:
    ACK 31cce4a
  naumenkogs:
    ACK 31cce4a

Tree-SHA512: 8aea92c53c1911a0ac36fe9e3a24d37d83e7d9b40a16f0832bfa7a719328697621e3f94a5dc80d1840e7ae705e0c3aab7a3df7064986e1e53a4a4114adf078a8
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
31cce4a doc: update `BroadcastTransaction` comment (ismaelsadeeq)

Pull request description:

  `BroadcastTransaction` is also called by `submitpackage` RPC.

  All transactions that are accepted into the mempool post package processing are broadcasted to peers individually here
  https://github.com/bitcoin/bitcoin/blob/ea4ddd8652d9dd1e7698e2a6f84c606cf24a2e3e/src/rpc/mempool.cpp#L926

  It's not maintainable to list all the callers of a function.

ACKs for top commit:
  stickies-v:
    ACK 31cce4a
  kristapsk:
    ACK 31cce4a
  naumenkogs:
    ACK 31cce4a

Tree-SHA512: 8aea92c53c1911a0ac36fe9e3a24d37d83e7d9b40a16f0832bfa7a719328697621e3f94a5dc80d1840e7ae705e0c3aab7a3df7064986e1e53a4a4114adf078a8
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 24, 2024
8bf1d06 Merge bitcoin#29308: doc: update `BroadcastTransaction` comment (glozow)
2a77808 Merge bitcoin-core/gui#789: Avoid non-self-contained Windows header (Hennadii Stepanov)
da371b8 Merge bitcoin#28870: depends: Include `config.guess` and `config.sub` into `meta_depends` (fanquake)
2e41562 Merge bitcoin#29219: fuzz: Improve fuzzing stability for ellswift_roundtrip harness (fanquake)
b091329 Merge bitcoin#29211: fuzz: fix `connman` initialization (Ava Chow)
df42d41 Merge bitcoin#29200: net: create I2P sessions using both ECIES-X25519 and ElGamal encryption (fanquake)
4cdd1a8 Merge bitcoin#29172: fuzz: set `nMaxOutboundLimit` in connman target (fanquake)
97012ea Merge bitcoin#28962: doc: Rework guix docs after 1.4 release (fanquake)
c70ff5d Merge bitcoin#28844: contrib: drop GCC MAX_VERSION to 4.3.0 in symbol-check (fanquake)
e6f19e7 Merge bitcoin#29068: test: Actually fail when a python unit test fails (fanquake)
75e0334 Merge bitcoin#28989: test: Fix test by checking the actual exception instance (Andrew Chow)
8cd85d3 Merge bitcoin#28852: script, assumeutxo: Enhance validations in utxo_snapshot.sh (Ryan Ofsky)
fd2e88d Merge bitcoin#26077: guix: switch from `guix environment` to `guix shell` (fanquake)
02741a7 Merge bitcoin#28913: coins: make sure PoolAllocator uses the correct alignment (fanquake)
dfd53da Merge bitcoin#28902: doc: Simplify guix install doc, after 1.4 release (fanquake)

Pull request description:

  ## Issue being fixed or feature implemented
  Batch of trivial backports

  ## What was done?
  See commits

  ## How Has This Been Tested?
  built locally; large combined merge passed tests locally

  ## Breaking Changes
  Should be none

  ## 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
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 8bf1d06
  knst:
    utACK 8bf1d06

Tree-SHA512: 506273e5a188f9ca74edf656e3cd338992192e6e97f68c89fc43e34be20fb7f211b48e4dfa8693727839a7920da8284509413c722f55774a428939c296dad517
@bitcoin bitcoin locked and limited conversation to collaborators Jun 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants