Skip to content

invtxblockSplitinvtxblock #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: splitinvtxblock
Choose a base branch
from
Open

invtxblockSplitinvtxblock #77

wants to merge 5 commits into from

Conversation

gmaxwell
Copy link

This will avoid sending more pointless INVs around updates, and
prevents using filter updates to timetag transactions.

gmaxwell and others added 4 commits April 19, 2016 19:51
@gmaxwell @sipa
Eliminate TX trickle bypass, sort TX invs for privacy and priority.
613b754 
Previously Bitcoin would send 1/4 of transactions out to all peers
 instantly.  This causes high overhead because it makes >80% of
 INVs size 1.  Doing so harms privacy, because it limits the
 amount of source obscurity a transaction can receive.

These randomized broadcasts also disobeyed transaction dependencies
 and required use of the orphan pool.  Because the orphan pool is
 so small this leads to poor propagation for dependent transactions.

When the bypass wasn't in effect, transactions were sent in the
 order they were received.  This avoided creating orphans but
 undermines privacy fairly significantly.

This commit:
 Eliminates the bypass. The bypass is replaced by halving the
  average delay for outbound peers.

 Sorts candidate transactions for INV by their topological
  depth then by their feerate (then hash); removing the
  information leakage and providing priority service to
  higher fee transactions.

 Limits the amount of transactions sent in a single INV to
  7tx/sec (and twice that for outbound); this limits the
  harm of low fee transaction floods, gives faster relay
  service to higher fee transactions. The 7 sounds lower
  than it really is because received advertisements need
  not be sent, and because the aggregate rate is multipled
  by the number of peers.
@sipa
Split up and optimize transaction and block inv queues
1fbda60
@sipa
Handle mempool requests in send loop, subject to trickle
2dd8219 
By eliminating queued entries from the mempool response and responding only at
trickle time, this makes the mempool no longer leak transaction arrival order
information (as the mempool itself is also sorted)-- at least no more than
relay itself leaks it.
@sipa
Return mempool queries in dependency order
ce1d237
@gmaxwell
Copy link
Author

I think this is the right thing to do. It needs a locking fix in fRelayTxes (broken in master), and removal of the feerate from RelayTransaction's prototype; but I'm not going to finish it if you don't think we should do this.

@sipa sipa force-pushed the splitinvtxblock branch 2 times, most recently from e3bacef to d4a6f40 Compare April 20, 2016 08:36
@sipa
Copy link
Owner

sipa commented Apr 20, 2016

Looks good.

@gmaxwell
Move bloom and feerate filtering to just prior to tx sending.
7f73f58 
This will avoid sending more pointless INVs around updates, and
 prevents using filter updates to timetag transactions.

Also adds locking for fRelayTxes.
@sipa sipa force-pushed the splitinvtxblock branch 2 times, most recently from cda0894 to b559914 Compare April 20, 2016 22:36
sipa pushed a commit that referenced this pull request Nov 18, 2020
Merge bitcoin-core/gui#96: Slight improve create wallet dialog
e7986c5 
ac64cec gui: create wallet: add advanced section (Sjors Provoost)
c99d6f6 gui: create wallet: name placeholder (Sjors Provoost)
5bff825 [gui] create wallet: smarter checkbox toggling (Sjors Provoost)

Pull request description:

  Previously only users who needed a second wallet had to use to the create wallet dialog. With the merge of bitcoin#15454 now all new users have to. I don't think it was user-friendly enough for that.

  <img width="403" alt="Schermafbeelding 2020-09-18 om 09 41 44" src="https://user-images.githubusercontent.com/10217/93574129-52ef9680-f998-11ea-9a6f-31144f66d3bf.png">

  This PR makes a few simple improvements so that new users don't have to think too much:

  <img width="369" alt="Schermafbeelding 2020-10-15 om 16 45 22" src="https://user-images.githubusercontent.com/10217/96145959-0c914700-0f06-11eb-9526-cf447d841d7a.png">

  It's lightly inspired by #77. It would be better if those changes made it into the upcoming release, but this PR is a good start imo.

  * wallet encryption is no longer checked by default, because such a change in the default needs a separate discussion (fwiw, I suspect it increases the number of users losing access to coins)
  * watch-only and descriptor wallet stuff is moved to advanced, so new users know they can safely ignore these check boxes
  * bonus: when you click on "disable private keys" it disables encrypt wallet and checks blank wallet
  * label changes: see screenshot
  * tooltip changes: see code diff

  Note that a blank wallet name isn't allowed in the dialog; I haven't addressed that.

  _Update 2020-10-30_, dropped the new strings for now:
  <img width="450" alt="Schermafbeelding 2020-10-30 om 11 26 55" src="https://user-images.githubusercontent.com/10217/97694591-1b99fc80-1aa3-11eb-8b85-e19f1ad5add4.png">

ACKs for top commit:
  fjahr:
    Tested ACK ac64cec
  jonatack:
    re-ACK ac64cec, per `git diff d393708 ac64cec` only change since my last review is improving the placeholder from "MyWallet" to "Wallet" and dropping the last commit. Tested creating a dozen wallets in signet with different combinations of options and then verifying/comparing their characteristics in the console with getwalletinfo. My remaining caveats are (1) the need for less user surprise by either (a) improving the user info or (b) with less auto-(un)selecting as mentioned in bitcoin-core/gui#96 (comment) and (2) I prefer the "Encrypt private keys" and "Watch-only" wording and descriptions below over the current ones; hopefully these can be addressed in a follow-up.
  hebasto:
    re-ACK ac64cec
  ryanofsky:
    Code review ACK ac64cec. Only changes since last review are tweaking placeholder text and dropping "allow nameless" commit

Tree-SHA512: a25f84eb66ee4f99af441d73e33928df9d9cf592177398ef48f0037f5913699e47a162cf1301c83b34501546d43ff4ae12607fd078c5c03b92f573bf7604a9f2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gmaxwell @sipa