Skip to content
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.

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

add feefilter documentation #1384

Merged
merged 2 commits into from Mar 16, 2017
Merged

Conversation

jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Oct 5, 2016

This PR adds documentation for the feefilter message type, added in Bitcoin Core V0.13.0.


| Bytes | Name | Data Type | Description
|-------|---------|-----------|---------------
| 8 | feerate | uint64_t | *Added in protocol version 70013 as described by BIP133.* <br><br>The fee rate (in Satoshis per kilobyte) under which transactions should not be relayed to this peer.
Copy link
Contributor

@Mirobit Mirobit Dec 9, 2016

Choose a reason for hiding this comment

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

Added in protocol version 70013 as described by BIP133.

is a duplicate. Already mentioned on the top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Mirobit . You're right. I've removed the dupe.

@Mirobit
Copy link
Contributor

Mirobit commented Dec 20, 2016

utACK

@harding
Copy link
Contributor

harding commented Dec 22, 2016

Thanks, @jnewbery !

I just sent @jnewbery some suggested edits in jnewbery#1 . I also previewed the site after those edits and it LGTM. If those edits are merged, I give a tested ACK.

Here's a screenshot of that preview (edit: I just realized that the table looks broken---that's because I reduced the font size to make everything fit on one screen; it looks correct in the original):

2016-12-21-21_32_24_535332643

@jnewbery
Copy link
Contributor Author

Looks good to me. Thanks @harding!

@wbnns
Copy link
Contributor

wbnns commented Jan 27, 2017

@jnewbery Hello, thanks a bunch for submitting this (@harding thanks for helping!). Could you please rebase and resolve the conflicts? Once you've done this I'll schedule the PR be merged.

@wbnns
Copy link
Contributor

wbnns commented Mar 8, 2017

@jnewbery Heya! Just wanted to follow-up again to see if you wouldn't mind rebasing this so we could go ahead and get it on the site.

@jnewbery
Copy link
Contributor Author

jnewbery commented Mar 8, 2017

Apologies for the delay on this. Rebased.

@rosser2121
Copy link

rosser2121 commented Mar 8, 2017 via email

@wbnns
Copy link
Contributor

wbnns commented Mar 11, 2017

@jnewbery Thanks! @rosser2121 Here's more information on what a rebase is.

Unless others object, this will be merged on Sunday, March 12th.

The receiving peer may choose to ignore the message and not filter transaction
inv messages.

The fee filter is additive with bloom filters. If an SPV client loads a bloom filter and sends a feefilter message, transactions should only be relayed if they pass both filters.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think at this point it should be noted that in the case of blocks, feefilter is not additive with bloom filters.

Copy link
Contributor

Choose a reason for hiding this comment

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

More specifically, I'd perhaps add "Both block and filtereredblock messages are unaffected by fee filtering."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you mean "Both block and filteredblockMerkleBlock messages..." (filteredblock is an inv type rather than a message).

If we add this, I think it should be more general, ie "feefilter has no impact on block propagation or responses to getdata messages", since it also won't affect Headers or CompactBlock messages. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, that's what I meant. Your suggestion seems fine to me too.

@jnewbery
Copy link
Contributor Author

@schildbach I've add a bit more explanation about feefilter interaction with block propagation and responses to getdata requests:

The fee filter is additive with bloom filters. If an SPV client loads a bloom
filter and sends a feefilter message, transactions should only be relayed if
they pass both filters.

Note however that feefilter has no effect on block propagation or responses to
getdata messages. For example, if a node requests a merkleblock from its peer
by sending a getdata message with inv type MSG_FILTERED_BLOCK and it has
previously sent a feefilter to that peer, the peer should respond with a
merkleblock containing all the transactions matching the bloom filter, even
if they are below the feefilter fee rate.

Let me know if you have any comments about that.

@harding @wbnns let me know if you're happy with this addition and I'll squash into a single commit.

@schildbach
Copy link
Contributor

Sounds fine to me.

@wbnns
Copy link
Contributor

wbnns commented Mar 16, 2017

@jnewbery Looks good, thanks!

@laanwj
Copy link
Contributor

laanwj commented Mar 16, 2017

Thanks for adding this!

jnewbery and others added 2 commits March 16, 2017 09:27
- Add annotated hex example

- Add search box entry and autocrossref entry

- Add Bitcoin Core 0.12/0.13 cross links
@jnewbery
Copy link
Contributor Author

commits squashed. This is ready for merging.

@wbnns
Copy link
Contributor

wbnns commented Mar 16, 2017

@jnewbery Thank you sir - will merge this as soon as tests pass. 👍

@wbnns wbnns merged commit 72e316a into bitcoin-dot-org:master Mar 16, 2017
@jnewbery jnewbery deleted the fee-filter branch March 16, 2017 14:30
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.

None yet

7 participants