Only support compact blocks with witnesses (
p2p) Jan 6, 2021
The PR branch HEAD was f8ff628 at the time of this review club meeting.
Compact blocks are a way to relay blocks across the Bitcoin P2P network with
reduced bandwidth usage. They can also reduce block propagation latency when
used in BIP152
BIP152 is the
specification for compact blocks.
BIP152 was originally developed and deployed before segwit was activated;
Bitcoin Core’s implementation was merged in
8068 and included in the v0.13.0
release. The specification was made extensible so that compact blocks could be
used for non-witness serialized blocks (version 1) and witness serialized
blocks (version 2).
activated in August 2017. Every version of Bitcoin Core since v0.13.1 supports
segwit and will fully validate blocks according to the BIP141 consensus
rules. To fully validate a block with segwit transactions, it must be
serialized with witnesses.
Version 1 (non-witness) compact blocks are therefore no longer useful to peers on the
network. This PR removes support for serving version 1 compact blocks.
How does using compact blocks save bandwidth? What data is not downloaded
when relaying blocks using compact blocks?
What is BIP152 high-bandwidth mode? How many of our peers can we choose to be
How do we choose which peers should be high-bandwidth peers? In which
function does that logic exist?
If a peer chooses us to be its high-bandwidth peer, how does it signal
that to us?
BIP152 states: “high-bandwidth mode permits relaying of CMPCTBLOCK messages
prior to full validation (requiring only that the block header is valid
before relay).” In which
PeerManager function do we relay compact blocks
to peers that have chosen us to be a high-bandwidth peer?
How is that
PeerManager function invoked? In which thread is it called?
1 18:00 <jnewbery> #startmeeting
3 18:00 <jnewbery> Hi folks. Welcome to the first Bitcoin Core PR Review Club of 2021 🎉
13 18:00 <joelklabo> good morning 👋
14 18:00 <anir> hey everyone!
15 18:00 <AnthonyRonning> Hi all!
18 18:00 <larryruane_> hi!
19 18:00 <jnewbery> I hope you all had a good break, and are feeling ready to learn even more about Bitcoin Core this year :)
20 18:00 <jnewbery> feel free to say hi to let everyone know you're here
21 18:00 <sunon> Hey!! Elle yay!
26 18:01 <elle> hey sunon! :)
28 18:01 <michaelfolkson> hi
29 18:01 <jnewbery> Is anyone here for the first time?
31 18:01 <AnthonyRonning> first time here
33 18:01 <jnewbery> Welcome AnthonyRonning! Thanks for joining us
34 18:01 <keyboardwarrior> hi :)
35 18:01 <sunon> Trying via a matrix.org proxy for the first time. Let’s see how this goes
36 18:01 <fodediop> Welcome Anthony! 🎉
37 18:01 <olympics> welcome AnthonyRonning
38 18:02 <AnthonyRonning> 🙌
39 18:02 <Caralie> hi :) Welcome Anthony!
41 18:02 <jnewbery> A couple of reminders on the format: i have some prepared questions to guide the conversation, but feel free to jump in at any time. Don't worry if you have a question about something that we covered earlier or whatever.
42 18:03 <jnewbery> also, don't ask to ask. Just jump right in.
43 18:03 <jnewbery> we're all here to learn and all questions are welcome
44 18:03 <jnewbery> ok, onto the PR. Who had a chance to review it? (y/n)
48 18:03 <AnthonyRonning> y
57 18:03 <theStack> 0.5 y (did just skim over the commits w/o detailled code review)
59 18:04 <Legousk> Where can I see beforehand which one will be reviewed?
60 18:04 <jnewbery> nice. No problem if you didn't have time this week
61 18:04 <elle> 0.5 y (spent most time on context)
65 18:04 <jnewbery> Legousk: they're announced on the website, usually the Friday before.
67 18:04 <jnewbery> any initial thoughts about the PR? Concept ACK/NACKs?
70 18:05 <AnthonyRonning> concept ack, love removing code
72 18:05 <sunon> Concept ACK
73 18:05 <jnewbery> YES! Removing code is the best!
74 18:05 <emzy> concept ack, not needed anymore.
75 18:05 <pinheadmz> ack yes
76 18:05 <jarolrod> concept ack makes sense
77 18:05 <thomasb06> not acquainted with the code enough
79 18:05 <joelklabo> ack 👍
80 18:05 <theStack> concept ack!
81 18:05 <michaelfolkson> Concept ACK but remind me how many old versions it is Core policy to support?
82 18:05 <anir> Concept ack, streamlines the codebase
84 18:06 <fodediop> Concept but still trying to digest the codebase
86 18:06 <michaelfolkson> 0.13 is pretty old
87 18:06 <jnewbery> michaelfolkson: that's a very broad question. There are lots of different versionings (eg p2p protocol, wallet, file format, ...)
88 18:06 <jnewbery> First question. How does using compact blocks save bandwidth? What data is not downloaded when relaying blocks using compact blocks?
89 18:07 <ccdle12> the transactions aren't downloaded? it
90 18:07 <jarolrod> Compact Blocks only includes a short transaction id for each transaction contained within it. The node on the receiving end of a compact block must match a transaction id with a transaction in its mempool and reconstruct the full block. Since we are not sending all of the transaction data, we can reduce block relay bandwidth around ~90%
91 18:07 <sunon> Most of the actual transactions
92 18:07 <pinheadmz> full transaction data is not downloaded, just short IDs
94 18:07 <pinheadmz> in hopes the recipient has the tx data already
95 18:07 <joelklabo> transactions are assumed to be in the mempool already
97 18:07 <michaelfolkson> jnewbery: For this type of p2p change how many old versions should be supported?
98 18:07 <thomasb06> only the transactions missing are transmitted
99 18:07 <thomasb06> the block isn't
100 18:07 <emzy> We only send the block header and short IDs of the transactions. If the peer has all the transactions in the mempool it can reconstruct the block without additional data.
101 18:07 ℹ sdaftuar_ is now known as sdaftuar
102 18:08 <sunon> Some transaction predicted to not be known to receiver are transmitted too
103 18:08 <glozow> PrefilledTransactions for txns we expect the peer to be missing, e.g. the coinbase tx
104 18:08 <pinheadmz> michaelfolkson i think an important point is that no one will ever need version 1 compact blocks because the last non witness block was so long ago, and compacts only work for recent blocks
105 18:08 <Legousk> jonatack: I'm not on twitter, but will follow it tho
106 18:08 <emzy> pinheadmz: exactly.
107 18:08 <pinheadmz> even non upgraded nodes (non segwit) it wont affect them, they arent getting v1 compacts any more anyway
108 18:08 <jnewbery> Lots of great answers there. Yes, when the node sends a CMPCTBLOCK it'll include shortids for the transactions, and some prefilled transactions, including the coinbase
109 18:08 <michaelfolkson> pinheadmz: If you are running 0.13 and want to use compact blocks?
110 18:08 <pinheadmz> i still wont send them to you
111 18:08 <pinheadmz> the last non witness block was SO LONG ago, you definitely dont have those txs in your mempool :-)
112 18:09 <pinheadmz> and having tx in the mempool is required for compact blocks to be useful
113 18:09 <jnewbery> michaelfolkson: if you're running 0.13.0, then you're not enforcing all consensus rules
114 18:09 <sdaftuar> pinheadmz: that's not exactly right
115 18:09 <sipa> michaelfolkson: if you're running 0.13.0, yes; not 0.12.x (because it has no compact blocks), and not 0.13.1+ (because it has segwit activated)
116 18:10 <sdaftuar> pinheadmz: currently an 0.13.0 node could attempt to use compact block relay, and it would get short-ids based on txid rather than wtxid
117 18:10 <sunon> So it’s really only very specifically 0.13.0 that’s the weird case?
118 18:10 <olympics> so it only affects node IFF they run 0.13.0
120 18:10 <sipa> olympics: i believe so
121 18:10 <sdaftuar> pinheadmz: but you are right that reconstruction would always require a roundtrip because those witness transactions would be nonstadnard to an 0.13.0 node
122 18:10 <sdaftuar> however, the protocol would work
123 18:10 <sipa> (or any other codebase that behaves similarly)
124 18:10 <pinheadmz> sdaftuar i see ok thanks
125 18:10 <jnewbery> or theoretically some other implementation that implemented BIP 152 but not segwit
126 18:10 <jnewbery> maybe some old version of an alternative implementation
127 18:11 <sunon> Oh yes of course
128 18:11 <norisg> did the block structure also change with segwit, you are talking about "non witness blocks", non witness blocks have no segwit transactions in it?
129 18:11 <pinheadmz> sdaftuar non standard TXs require a roundtrip? even if the tx is in the mempool (by txid, not wtxid) ?
130 18:11 <sdaftuar> michaelfolkson: importantly, this doesn't fundamentally break blockrelay to 0.13.0 nodes -- just won't use compact blocks for it
131 18:11 <sdaftuar> so to your question about "supporting" older nodes, this change wouldn't cause 0.13.0 nodes to fall out of consensus
132 18:11 <jnewbery> norisg: yes, the serialization of blocks changed with segwit. See BIP 144 for details
133 18:12 <larryruane_> so a segwit block could happen to have no segwit transactions?
134 18:12 <olympics> sdaftuar the worst effect would be, marginally longer txn propagation to nodes on 0.13.0?
135 18:12 <sdaftuar> pinheadmz: well non-standard transactions are (by definition) not in the mempool -- so reconstruction would fail
136 18:12 <sdaftuar> olympics: yep
138 18:12 <sdaftuar> block propagation
139 18:12 <sipa> larryruane_: then by definition it is not a segwit block :)
140 18:12 <pinheadmz> sdaftuar ah yes yes ty
141 18:13 <sdaftuar> pinheadmz: (well we also have the extra pool, too -- but that is relatively small)
142 18:13 <olympics> sdaftuar should I have said block propagation not txn propagation ?
143 18:13 <sdaftuar> olympics: yes
144 18:14 <olympics> bc the shortID expedites relaying the txns in the block rather than shipping out txns the peer may already have
145 18:14 <jnewbery> Next question: What is BIP152 high-bandwidth mode? How many of our peers can we choose to be high-bandwidth peers?
146 18:14 <ccdle12> 3 peers as high bandwith, high-bandwith means the nodes send compacted blocks by default
147 18:15 <emzy> We send the compact block without the peer asking for it, also before we validated it ourself. Max is 3 peers.
148 18:15 <thomasb06> 2 max
149 18:15 <anir> peers send new block announcements with the short transaction IDs already via a cmpctblock message, and it’s enabled by setting the first boolean to 1 in a sendcmpct message
150 18:15 <AnthonyRonning> High bandwidth mode is available so that peers can receive blocks automatically and asap (`1.5 * RTT`), even before full block validation takes place.
151 18:15 <sunon> High bandwidth mode allows peers to send compact blocks without sending inv msg / asking permission
152 18:15 <willcl_ark> look like 3 peers
153 18:15 <jnewbery> olympics: tx relay (for unconfirmed transactions) is unaffected by this. compact blocks makes block propogation faster based on the observation that the receiver probably has most of the transactions in their mempool already
154 18:15 <pinheadmz> high bandwidth sends unannounced compcat block
155 18:15 <willcl_ark> from `MaybeSetPeerAsAnnouncingHeaderAndIDs`
156 18:15 <jarolrod> high bandwidth mode is when you send cmpct blocks without asking for permission, can lead to higher bandwidth because you can receive the same block more than once, but reduced latency because potentially less round trips
157 18:15 <norisg> we are removing code, but no functionality ?
158 18:16 <pinheadmz> i was confused by this at first bc if oyu look at bip152 graphic "low bandwiwdth" appears to have more messages
160 18:16 <thomasb06> high bandwidth mode means the nodes will only get the missing transactions
161 18:16 <lightlike> when it was introduced, why wasn't it decided to schedule compact blocks after segwit? Seems like a lot of effort for just one version 0.13.0
162 18:16 <sipa> norisg: wouldn't that be great? no, we are removing functionality: dropping support for the non-segwit version of compact blocks
163 18:16 <sunon> Also yeah only header needs to be validated before they’re sent in high bandwidth mode right?
164 18:16 <AnthonyRonning> one thing I was unsure of, bip152 says "Nodes MUST NOT send such sendcmpct messages to more than three peers" but sendcmpct can be for low bandwitdth too, so is the three limit total or high bandwidth?
165 18:16 <sipa> lightlike: things get merged when they're ready, and compact blocks did not require a softfork or anything
166 18:16 <sdaftuar> lightlike: segwit was also a big change, and it was unclear when it would be finally ready
167 18:16 <anir> upto 3 peers recommended I believe
168 18:17 <norisg> sipa because the probability of the block without segwit tx is aganist 0?
169 18:17 <theStack> it should not only affect 0.13.0 but also other higher versions that don't have activated segwit yet, right? (afair segwit came with v0.17, so there must be some versions v0.14.x and v0.15.x etc. with non-latest minor number that are affected)
170 18:17 <lightlike> ah ok, thanks!
171 18:17 <sipa> lightlike: so at the time it was included, it worked immediately on the network, and would keep doing so regardless of whether segwit activated or not (which turned out to be... uncertain for a while)
172 18:17 <sunon> I also believe I saw 3 peers max
173 18:17 <thomasb06> it's >= indeed
174 18:17 <sipa> norisg: no no
175 18:17 <sipa> norisg: the new protocol works even with non-segwit blocks
176 18:17 <jnewbery> ... or indeed if it would ever be activated. Segwit and compact blocks are indepedently good things, so it doesn't make sense to gate one on the other
177 18:17 <michaelfolkson> You just need one SegWit transaction for it to be a SegWit block
178 18:18 <sdaftuar> theStack: no, everything from 0.13.1 on is segwit aware/activated
179 18:18 <sipa> norisg: it should be less ambigious and say "segwit-supporting compact block version" and "non-segwit-supporting compact block version" perhaps
180 18:18 <theStack> sdaftuar: how so? in 0.14.0 there was no segwit
181 18:18 <sipa> theStack: there absolutely was
182 18:18 <sdaftuar> theStack: that is not true!
183 18:19 <theStack> ah, right
184 18:19 <sipa> theStack: wallet support for segwit came in 0.16 iirc, but that's unrelated
185 18:19 <ccdle12> i was curious what was the specific reason for a max of 3 high bandwith nodes?
186 18:19 <michaelfolkson> As pinheadmz said it must have been a long time since a mined block didn't have a single SegWit transaction
187 18:19 <norisg> sipa ok you you are saying that the "segwit-supporting compact block version" can also propagate non-segwit blocks?
188 18:19 <sunon> Is it not for DoS mitigation?
189 18:19 <jnewbery> theStack: the code for segwit was merged in bitcoin 0.13.1. Activation was later, but any node on 0.13.1 or higher can understand/validate/relay segwit blocks/txs
190 18:20 <theStack> for some reason i thought it was with 0.17, sorry :x
191 18:20 <sipa> norisg: correct
192 18:20 <jonatack> was afk, catching up, we can select up to 3, not 2, peers to be bip152 high-bandwidth
193 18:20 <norisg> and this pull request we are talking about implements "segwit-supporting compact block version"
194 18:20 <willcl_ark> ccdle12: as per BIP152: "Nodes MUST NOT send such sendcmpct messages to more than three peers, as it encourages wasting outbound bandwidth across the network."
195 18:21 <jnewbery> norisg: no. It removes support for non-segwit-supporting compact block version
196 18:21 <ccdle12> willcl_ark: thanks
197 18:21 <sipa> norisg: no; the current codebase supports both; what we're discussing is removing support for the "non-segwit-supporting compact block version"
198 18:21 <nR2Cncdm> Hello, my first time. Just lurking today. Hope to make an impact before I die!
199 18:21 <AnthonyRonning> that's what I'm confused about willcl_ark, doesn't low bandwidth send sendcmpct too?
200 18:21 <glozow> we can't tell if a node has more than 3, can we?
201 18:21 <nR2Cncdm> (i'm not expecting death btw)
202 18:21 <sipa> AnthonyRonning: yes, but with an extra roundtrip
203 18:21 <thomasb06> jonatack: yep, misread the code
204 18:21 <sipa> AnthonyRonning: the extra roundtrip means that the receiver only asks for the actual compact block once
206 18:22 <AnthonyRonning> sipa: so the three limit applies to low too? not just high?
208 18:22 <emzy> nR2Cncdm: good to hear. You will make an impect with the right mindset.
209 18:22 <sipa> AnthonyRonning: all the ones that are not hb are lb
212 18:23 <jnewbery> and that's exactly what Bitcoin Core implements
213 18:23 <troygior1hev> jonatack: nice!
214 18:23 <sipa> AnthonyRonning: the hb peers will just send the compact block without being asked for it; the lb peers will announce "hey i have a compact block for you, want it?" - and then the receiver gets to pick which of the lb peers to ask it from
215 18:23 <jonatack> or via getpeerinfo in master now that theStack's PR exposing these two fields was merged
216 18:23 <jnewbery> Next question (should be easy given the link above): How do we choose which peers should be high-bandwidth peers? In which function does that logic exist?
217 18:23 <AnthonyRonning> sipa: thanks!
218 18:23 <jonatack> troygior1hev: ty :)
219 18:23 <AnthonyRonning> High bandwidth mode is for peers that have a past performance in providing blocks quickly. I believe it exists in `MaybeSetPeerAsAnnouncingHeaderAndIDs`
220 18:23 <norisg> sipa: what was the reason why we had to implement it in the first place, but did't remove it immediatly when introducing "segwit-supporting compact block version"
221 18:24 <pinheadmz> jnewbery in the updated code, if i send you SENDCMPCT with version 1, the function just returns. So you will not send me compact blocks - is my node "ok with this"? Or do we disconnect from peers that dont send compact blocks after we ask them to ?
222 18:24 <sipa> norisg: because it was necessary at the time; segwit wasn't active on the network
223 18:24 <norisg> thx sipa
224 18:24 <jnewbery> norisg: because there's no rush, and potentially there could have been peers on 0.13.0 who still wanted to use compact blocks
225 18:25 <michaelfolkson> norisg: Why remove functionality for 0.13.0 nodes if Core current version is 0.14?
227 18:25 <thomasb06> the function is "connman.ForNode(.,.)"
228 18:25 <anir> I think peers be selected based on their past performance in providing blocks quickly
229 18:26 <willcl_ark> jnewbery: this is in src/net_processing.cpp#MaybeSetPeerAsAnnouncingHeaderAndIDs called by PeerManager::BlockChecked
230 18:26 <pinheadmz> MaybeSetPeerAsAnnouncingHeaderAndIDs sets `pfrom->m_bip152_highbandwidth_to = true;`
231 18:26 <jonatack> anir: right
232 18:27 <jonatack> for the peers our node chooses
233 18:27 <pinheadmz> high bandwidth peers are chosen if they are the first peer to inform us of a new block
234 18:27 <jnewbery> pinheadmz: Very good question. I think Bitcoin Core would be "ok with this", but it's maybe worth a quick check in the Bitcoin Core 0.13.0 code. If it's not ok with it, then maybe a polite disconnect would be better.
235 18:28 <AnthonyRonning> if I forked core to connect to all my peers with high bandwith mode, is that allowed? can the network see I have connected to more than 3?
236 18:29 <michaelfolkson> AnthonyRonning: No feel free!
237 18:29 <jnewbery> To those that answered BlockChecked -> MaybeSetPeerAsAnnouncingHeaderAndIDs : you're right!
238 18:29 <sdaftuar> AnthonyRonning: no one is in charge...
239 18:29 <willcl_ark> AnthonyRonning: I don't think anyone except you would know, it's just bad practice
240 18:29 <sipa> AnthonyRonning: yes, you'll primarily be wasting your own bandwidth
241 18:29 <sunon> I don’t think that would be any issue
242 18:29 <thomasb06> arg...
243 18:29 <emzy> AnthonyRonning: I think there is no way. But what will you gain. Execpt of more bandwith incomming?
244 18:29 <jnewbery> AnthonyRonning: no-one can stop you, but the BIP says you shouldn't: Nodes MUST NOT send such sendcmpct messages to more than three peers, as it encourages wasting outbound bandwidth across the network.
245 18:30 <AnthonyRonning> 👍
246 18:30 <glozow> AnthonyRonning: If your question is whether nodes detect/punish this behavior I don't think they do
247 18:30 ⚡ emzy has unlimited bandwith on his node. Be my guest :)
248 18:30 <glozow> how would they 🤔
249 18:30 <AnthonyRonning> glozow: yeah!
250 18:30 <norisg> does the network equally distribute the high bath width connection, lets say we have 5 nodes on the network and each one of them is connected to the same other 3 in high band with, how do we get the other one in
251 18:30 <jnewbery> Any questions about BlockChecked() or MaybeSetPeerAsAnnouncingHeaderAndIDs()?
252 18:30 <sunon> I mean it’s ok to play a little haha
253 18:31 <jnewbery> How about this question: "Where does BlockChecked() get called from? On which thread?"
254 18:31 <ccdle12> ThreadMessageHandler
255 18:32 <ccdle12> ProcessNewBlock I think
256 18:33 <jnewbery> ccdle12: yes, it can be called from ProcessNewBlock
257 18:33 <jnewbery> it can also be called from ConnectTip, if we tried to connect the block, but it failed at that point (e.g. if it spent coins that didn't exist)
258 18:34 <larryruane_> thread b-loadblk? (validation)
259 18:34 <jnewbery> And yes, those functions will almost always be called on ThreadMessageHandler
260 18:34 <jnewbery> larryruane_: I think you might be right that it could also be on that thread
261 18:35 <norisg> ccdle12 which codefile and line ?
262 18:35 <jnewbery> the interesting thing to note here is that CheckBlock is one of the two validation interface callbacks that are called synchronously. Does anyone know the other one?
263 18:35 <glozow> where can I find all the threads? grep for std::thread ?
264 18:35 <ccdle12> norisg: net.cpp::CConnman::ThreadMessageHandler() and net_processing.cpp:ProcessMessage()
265 18:35 <norisg> thx ccdle12
266 18:36 <troygior1hev> This is useful for anyone wanting to explore the different threads:
267 18:36 <ccdle12> norisg: ProcessMessage() is hugh fyi lol
269 18:36 <glozow> <troygior1hev> big thank 🙏
270 18:36 <larryruane_> glozow I think a good way to do it actually is to run the debugger, set a bp, then see which thread you're on (info threads helps)
271 18:37 <jnewbery> troygior1hev: good tip!
272 18:37 <glozow> larryruane_ oo nice!
273 18:37 <jnewbery> Also, try running with -logthreadnames and look at your debug log
274 18:37 <jonatack> doc/developer-notes.md has a section on the threads
275 18:38 <pinheadmz> jnewbery tracing through net_processing I think the requesting node doesnt care if it gets a block instead of a compact block. Even if the inv has type MSG_CMPCT_BLOCK, the remote node just tests for IsGenBlkMsg which includes all block tpe messages, and then decides later to send compact or not
276 18:38 <jnewbery> especially with the validation category enabled, because then you can see the validation interface callbacks being set and fired
277 18:38 <jonatack> troygior1hev: what you wrote ^
278 18:38 <jnewbery> pinheadmz: thanks! Quick work
279 18:39 <jnewbery> ok, we can come back to the validation interface in a bit. Let's move on to the next question. If a peer chooses us to be its high-bandwidth peer, how does it signal that to us?
280 18:39 <emzy> Sending sendcmpct with first byte set to1 (true).
281 18:39 <theStack> by sending the SENDCMPCT message with its payload byte set to 1
282 18:39 <AnthonyRonning> sending a 1 in the `sendcmpct` command
283 18:39 <norisg> how do we test this compact block feature, there should already be test files I guess
284 18:40 <jnewbery> yes. exactly right. The first byte of sendcmpct is for high bandwidth mode.
285 18:40 <ccdle12> norisg: test/functional/p2p_compactblocks.py
286 18:40 <michaelfolkson> norisg: There is already a functional test which is changed in the PR
287 18:40 <jnewbery> norisg: good question. Take a look at the commits in the PR and see which functional test is being updated
288 18:40 <theStack> norisg: see test/functional/p2p_compatblocks.py
289 18:40 <theStack> *p2p_compactblocks.py
290 18:41 <jnewbery> Next question. BIP152 states: “high-bandwidth mode permits relaying of CMPCTBLOCK messages prior to full validation (requiring only that the block header is valid before relay).” In which PeerManager function do we relay compact blocks to peers that have chosen us to be a high-bandwidth peer?
291 18:41 <AnthonyRonning> I think `NewPoWValidBlock` , determined by the `m_sendcmpct_hb` flag
292 18:41 <jarolrod> PeerManager::NewPOWValidBlock line 1250 of src/net_processing.cpp
293 18:42 <jnewbery> AnthonyRonning jarolrod: exactly right. It's in NewPOWValidBlock(). Can anyone tell me more about NewPOWValidBlock? Where is it called from?
294 18:43 <ccdle12> AcceptBlock()?
296 18:44 <AnthonyRonning> not sure the thread info though, haven't reviewed the threads doc yet
297 18:44 <jnewbery> yeah, AcceptBlock() has this call: `GetMainSignals().NewPoWValidBlock(pindex, pblock)`. What does that mean?
298 18:44 <norisg> why is propagation more important than verification
299 18:45 <larryruane_> Is it because that function is going to run in a different thread?
300 18:45 <ccdle12> actually that's something I was studying earlier, GetMainSignals() gets called in init.cpp, so is it like a main thread in a way?
301 18:45 <jnewbery> larryruane_: good guess. You're almost there
302 18:45 <larryruane_> (I meant the GetMainSignals)
303 18:45 <jarolrod> norisg: block propagation is important to prevent network splits
304 18:46 <sipa> norisg: it's not; but verification takes time, and latency on the network is important, so we want blocks to be able to propagate without suffering a delay from validation at every hop
305 18:46 <sipa> ccdle12: validation.cpp and net_processing.cpp together used to be main.cpp; i think that's where the name comes from
306 18:47 <ccdle12> sipa: aahh interesting, thanks
307 18:47 <larryruane_> also we don't want to give anyone an incentive to run a modified client
308 18:47 <jnewbery> ok, GetMainSignals is the way that validation fires a validation interface notification
309 18:47 <glozow> at this point though we've already verified the PoW, which would be very hard to fake, so it's not as risky right?
310 18:48 <larryruane_> john could you elaborate a bit?
311 18:48 <emzy> I think a valid Block header is enough to be relayed. It is enough work to come up with one.
312 18:48 <norisg> sipa: how long do we allow peers to send us false blocks until we close the connection
313 18:48 <jnewbery> other components subscribe to those notifications. Look for any class that inherits the CValidationInterface interface to see examples
314 18:48 <norisg> in high band with mode
315 18:48 <sdaftuar> glozow: yep the PoW prevents DoS. however it's worth looking at what happens if a block is invalid
316 18:48 <sipa> norisg: it is very expensive for them to do so, due to PoW
317 18:49 <sipa> (even unvalidated blocks must pass PoW)
318 18:49 <norisg> sipa perfect thanks
319 18:49 <jnewbery> many of those notifications are asynchronous (i.e. validation sends them, and then the subscibers get notified by a background thread), but there are a couple that are synchronous (i.e. the subscribers are called directly by the thread in validation)
320 18:49 <jnewbery> BlockChecked was one synchronous callback. What's the other?
322 18:51 <jonatack> jnewbery: i'd gladly review a commit adding that info in a GetMainSignals() doxygen doc in validationinterface.h
323 18:51 <pinheadmz> sdaftuar glozow related: i dont see in net_processing how we deal with unsolicited BLOCK messages? is that not a DoS risk?
324 18:52 <jnewbery> look for "called on a background thread"
325 18:52 <AnthonyRonning> ah so NewPoWValidBlock is sync too then?
327 18:53 <jnewbery> AnthonyRonning: correct
328 18:53 <jnewbery> why do you think that is?
329 18:53 <larryruane_> ChainStateFlushed() also
330 18:53 <glozow> pinheadmz: I suppose we check blocks in an order that prioritizes failing early
331 18:53 <jnewbery> ChainStateFlushed is asynchronous
332 18:54 <glozow> i.e. first non-contextual and look at PoW before doing any expensive verification
333 18:54 <AnthonyRonning> does BlockChecked not call NewPoWValidBlock? So they'd both be sync?
334 18:54 <pinheadmz> glozow ah yeah i thikn i see in CChainState::AcceptBlock() if we didnt request the block we check chainwork in the header before anything
335 18:54 <jnewbery> AnthonyRonning: No. NewPOWValidBlock is called by AcceptBlock()
336 18:55 <glozow> i think we do that for blocks we did request too, right?
337 18:55 <jnewbery> The reason it's important is that we want to send out the high-bandwidth compact blocks as quickly as possible
338 18:55 ℹ sanketcell_ is now known as sanketcell
339 18:55 <larryruane_> Why is there such a thing as synchronous notifications? How is that different from just a normal function call?
340 18:55 <pinheadmz> glozow sorry we just check against nMinimumChainWork
341 18:55 <jnewbery> so we immediately (synchronously) call the NewPoWValidBlock() callback in net_processing, which sends the cmpctblock message out immediately
342 18:56 <jnewbery> larryruane_: validation doesn't hold a reference/pointer to peermanager. Everything that peermanager gets from validation is through the validation interface
343 18:56 <AnthonyRonning> interesting, so do async notifications go through a queue of some sort? And calling it syncronously skips that?
344 18:57 <jnewbery> AnthonyRonning: basically yes, async notifications are all serviced on the background scheduler thread
345 18:57 <nehan> jnewbery: so the documentation that NewPoWValidBlock is synchronous vs asynchronous is the fact that it does *not* have "Called in a background thread" in its defn comments in validationinterface.h?
346 18:57 <nehan> how else might one confirm that?
347 18:57 <jnewbery> whereas synchronous notifications are just regular function calls by the thread that's doing the validation
348 18:58 <AnthonyRonning> gotcha, very cool, thanks
349 18:58 <nehan> just that its function doesn't call ENQUEUE_AND_LOG_EVENT?
350 18:58 <jnewbery> nehan: that's the documentation, yes! You could look in validationinterface.cpp to see how those notifications are invoked, or you could run a node with thread debug logs enabled and validation category debug logs enabled and see which threads are servicing the validation interface callbacks
351 18:59 <jnewbery> We've covered the last question already (How is that PeerManager function invoked? In which thread is it called?)
352 18:59 <jnewbery> And that's about time.
353 19:00 <jnewbery> Next week, dhruv is hosting on PR 19825. We'll have notes up by Friday.
354 19:00 <jnewbery> Thanks everyone!
355 19:00 <jnewbery> #endmeeting
356 19:00 <pinheadmz> thanks jnewbery !
357 19:00 <theStack> thanks for hosting jnewbery
358 19:00 <AnthonyRonning> thank you!
359 19:00 <troygior1hev> thanks jnewbery!
360 19:00 <olympics> thanks jnewbery
361 19:00 <norisg> thank you !
362 19:00 <ccdle12> thanks jnewbery
363 19:00 <thomasb06> thanks jnewbery
364 19:00 <emzy> Thank you jnewbery and all others. Happy, healthy and productive 2021 to you all.
365 19:00 <lightlike> thanks jnewbery
366 19:00 <glozow> thanks jnewbery!
367 19:00 <fodediop> Thanks jnewbery!
368 19:00 <larryruane_> thank you all, this was great!
369 19:00 <anir> Thanks jnewbery and everyone else!
370 19:01 <jarolrod> thanks