--- Log opened Wed Oct 27 00:00:15 2021
00:26 -!- Kaizen_Kintsugi [~Kaizen_Ki@node-1w7jr9yi65te7aswxjyf8efmc.ipv6.telus.net] has quit [Read error: Connection reset by peer]
00:27 -!- Kaizen_Kintsugi [~Kaizen_Ki@node-1w7jr9yi65te7aswxjyf8efmc.ipv6.telus.net] has joined #bitcoin-core-pr-reviews
04:12 -!- Netsplit *.net <-> *.split quits: r-ush, lsilva_, stick
04:15 -!- Netsplit over, joins: lsilva_, stick, r-ush
04:44 -!- jeremyrubin5 [~jeremyrub@ec2-44-199-24-18.compute-1.amazonaws.com] has joined #bitcoin-core-pr-reviews
04:45 -!- dr-orlovsky [~dr-orlovs@31.14.40.18] has joined #bitcoin-core-pr-reviews
04:45 -!- amiti_ [sid373138@id-373138.lymington.irccloud.com] has joined #bitcoin-core-pr-reviews
04:46 -!- waxwing [~waxwing@193.29.57.116] has joined #bitcoin-core-pr-reviews
04:46 -!- pinheadmz_ [~pinheadmz@hns-contributor.dev] has joined #bitcoin-core-pr-reviews
04:48 -!- djinni`_ [~djinni@static.38.6.217.95.clients.your-server.de] has joined #bitcoin-core-pr-reviews
04:52 -!- Netsplit *.net <-> *.split quits: amiti, pinheadmz, djinni`, waxwing_, dr_orlovsky, jeremyrubin
04:52 -!- jeremyrubin5 is now known as jeremyrubin
04:52 -!- amiti_ is now known as amiti
06:55 -!- dr-orlovsky [~dr-orlovs@31.14.40.18] has quit [Ping timeout: 260 seconds]
07:05 -!- awesome__ [~awesome_d@61-228-97-247.dynamic-ip.hinet.net] has joined #bitcoin-core-pr-reviews
07:05 -!- awesome__ [~awesome_d@61-228-97-247.dynamic-ip.hinet.net] has quit [Client Quit]
07:06 -!- awesome__ [~awesome_d@61-228-97-247.dynamic-ip.hinet.net] has joined #bitcoin-core-pr-reviews
07:06 -!- awesome__ [~awesome_d@61-228-97-247.dynamic-ip.hinet.net] has quit [Client Quit]
07:09 -!- awesome_doge [~awesome_d@61-228-97-247.dynamic-ip.hinet.net] has joined #bitcoin-core-pr-reviews
07:10 -!- awesome_doge [~awesome_d@61-228-97-247.dynamic-ip.hinet.net] has quit [Client Quit]
07:24 -!- awesome_doge [~awesome-d@2001:470:69fc:105::1:252c] has joined #bitcoin-core-pr-reviews
07:58 -!- davidbak [~DavidBaki@c-174-61-163-5.hsd1.wa.comcast.net] has joined #bitcoin-core-pr-reviews
08:00 -!- DavidBakin [~DavidBaki@c-174-61-163-5.hsd1.wa.comcast.net] has quit [Ping timeout: 264 seconds]
08:00 -!- b10c [uid500648@id-500648.ilkley.irccloud.com] has joined #bitcoin-core-pr-reviews
08:03 -!- schlamarcel [~schlamarc@2a02:8109:b540:5aa1::c8] has joined #bitcoin-core-pr-reviews
09:02 -!- luke-jr [~luke-jr@user/luke-jr] has quit [Read error: Connection reset by peer]
09:04 -!- luke-jr [~luke-jr@user/luke-jr] has joined #bitcoin-core-pr-reviews
09:15 -!- z9z0b3t1c [z9z0b3t1c@gateway/vpn/protonvpn/z9z0b3t1c] has joined #bitcoin-core-pr-reviews
09:18 -!- z9z0b3t1c [z9z0b3t1c@gateway/vpn/protonvpn/z9z0b3t1c] has quit [Quit: Lost terminal]
09:19 -!- z9z0b3t1c [z9z0b3t1c@gateway/vpn/protonvpn/z9z0b3t1c] has joined #bitcoin-core-pr-reviews
09:24 -!- hernanmarino [~hernanmar@OL121-235.fibertel.com.ar] has joined #bitcoin-core-pr-reviews
09:27 -!- hernanmarino [~hernanmar@OL121-235.fibertel.com.ar] has quit [Client Quit]
09:27 -!- hernanmarino [~hernanmar@OL121-235.fibertel.com.ar] has joined #bitcoin-core-pr-reviews
09:54 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has joined #bitcoin-core-pr-reviews
09:59 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
10:00 < glozow> #startmeeting
10:01 < Kaizen_Kintsugi> awww yisss
10:01 < Kaizen_Kintsugi> learning time :)
10:01 < Kaizen_Kintsugi> in the right place
10:01 < jnewbery> hi!
10:01 -!- lucasdcf [~lucasdcf@2804:431:c7d9:3516:16c0:9872:61fe:b8b5] has joined #bitcoin-core-pr-reviews
10:01 < glozow> Welcome to PR Review Club!
10:01 < davidbak> hi (lurking today)
10:01 < schmidty> hi
10:01 < hernanmarino> Hi ! 
10:01 -!- stickies-v [~stickies-@84-45-243-170.static.enta.net] has joined #bitcoin-core-pr-reviews
10:01 < glozow> Today we're looking at #22674: Mempool validation and submission for packages of 1 child + parents
10:01 < glozow> Notes in the usual place: https://bitcoincore.reviews/22674
10:01 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:a443:99fa:7edf:26be] has joined #bitcoin-core-pr-reviews
10:01 < glozow> More background on package mempool accept here: https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a
10:01 < SandipanDey[m]> hey there!
10:02 < stickies-v> hi everyone
10:02 < brunoerg> hi
10:02 < larryruane> hi
10:02 < lightlike> hi
10:02 < glozow> Did anyone get a chance to review the PR, look through the notes, or read the gist? How about a y/n for each?
10:02 < Kaizen_Kintsugi> y
10:02 < stickies-v> y/y/y
10:02 < jnewbery> y/y/y
10:02 < davidbak> n/y/y
10:03 < larryruane> n/y/n
10:03 < glozow> stickies-v: jnewbery: wowowow, extra bonus points! you'll surely ace the quiz
10:03 < hernanmarino> n/y/y
10:03 < Kaizen_Kintsugi> y/y/y sry
10:03 < glozow> Can anyone summarize what this PR does?
10:04 < Kaizen_Kintsugi> From what I can tell this creates an object for multiple dependent transactions
10:04 < Kaizen_Kintsugi> like to send transactions one after the other so things like fee bumping can be done on them easier in the future
10:04 -!- b10c [uid500648@id-500648.ilkley.irccloud.com] has quit [Quit: Connection closed for inactivity]
10:05 < hernanmarino> it defines mempool validation (criteria) for packages with one child 
10:05 < stickies-v> it adds validation logic for a specific type of package (1 child with multiple parents) so that it can be accepted into the mempool if valid, plus some improved error handling/messaging
10:05 < glozow> Kaizen_Kintsugi: partially correct, yes - it defines a package (group of dependent transactions) and implements package acceptance for packages of a specific topology. And it's in preparation for fee-bumping within packages in the future
10:06 < glozow> stickies-v: hernanmarino: right
10:06 < glozow> What does `IsChildWithParents()` check? How does it do this?
10:07 < larryruane> just to be sure, this concept of a package isn't exposed in any way to the outside world (outside the node) yet, right? No P2P support at all for it? (But will come later?)
10:07 < davidbak> one thing unclear to me is whether this is the _first_ change to B.C. dealing with packages or if packages already exist in some way.  The gist starts by saying this is for "packages consisting of multiple parents and 1 child" which leaves me wondering if there was a previous one, accepted, dealing with, say, 1 parent and 1 child.
10:07 < Kaizen_Kintsugi> cool cool, it seems to relate to the review club a few weeks ago about topoloogical sorting of transactions, this sounds like a pre-sort.
10:07 < glozow> larryruane: correct, no P2P support yet
10:07 < z9z0b3t1c> what problem does this change solve?
10:07 < stickies-v> it verifies that the submitted package has 1 child and that all inputs of that child are transactions that are either submitted in the package (parents) if they are unconfirmed, or transactions that are already confirmed and thus in the UTXO set
10:07 < glozow> but this is intended to naturally inform a P2P package
10:08 < Kaizen_Kintsugi> i would assume that ischildwith parents checks the mempool for the relevant transactions
10:08 < larryruane> if anyone here would like to play around with topological sort, `man tsort`
10:08 < glozow> I'm referring to `IsChildWithParents` as defined here https://github.com/bitcoin/bitcoin/blob/5ab8cb23e46152957d80f777310ec2493427a19e/src/policy/packages.h#L50
10:09 < Kaizen_Kintsugi> looks like it validates the package
10:09 < stickies-v> no sorry my earlier explanation is wrong - the UTXO checking happens somewhere else. IsChildWithParents just checks that all the parent txs are used as inputs for the child
10:10 < Kaizen_Kintsugi> and it orders them?
10:10 < glozow> stickies-v: yeah, you were close! that's indeed implemented in this PR, but the `IsChildWithParents` function only does context-free checking
10:11 < larryruane> stickies-v: ".. all inputs of that child are (...) in the package .." I don't think so, all parents don't need to be present in a package
10:11 < glozow> no validation is done at this point. we're just checking the package topology given the transaction objects themselves
10:11 < Kaizen_Kintsugi> oh cool, it validates the graph of parent child relationships?
10:12 < larryruane> I think any single tx in the mempool (regardless of parents) can be a legal package, right? 
10:12 < Kaizen_Kintsugi> looks like this package cant have a single transaction
10:12 < glozow> stickies-v's second answer is correct - we're making sure that  all of the transactions correspond to an input of the child (last transaction). though not all parents need to be present, as larryruane says
10:12 < larryruane> oh wait, no, sorry, there has to be at least 2 tx present
10:13 < glozow> yes, a package is at least 2 transactions
10:13 < glozow> next conceptual question: What criteria must a package meet in order to be considered a child-with-unconfirmed-parents package?
10:13 < glozow> Is it possible to verify without looking at the current chain? Is it possible to verify without looking at our mempool?
10:13 < larryruane> I think the doxygen comment in packages.h should say there must be at least 2
10:13 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews
10:14 < stickies-v> I think it's not possible to verify without looking at the current chain, but it is possible to verify without looking at the mempool since all confirmed parent transactions need to be in the package
10:14 < Kaizen_Kintsugi> damn, That's tough, i think you would have to look at the mempool
10:14 < stickies-v> *confirmed -> unconfirmed
10:14 < glozow> larryruane: noted :)
10:14 < glozow> stickies-v: correct!
10:14 < Kaizen_Kintsugi> what if the parents are in a different package?
10:15 < glozow> Kaizen_Kintsugi: we wouldn't need to look at the mempool, no
10:15 < glozow> er, how would the parents be in a different package?
10:15 < Kaizen_Kintsugi> someone put them their maliciously to screw with the system?
10:15 < Kaizen_Kintsugi> there*
10:15 < glozow> let's start by defining what a child-with-unconfirmed-parents package means
10:15 < glozow> can anyone tell us?
10:16 < Kaizen_Kintsugi> looks like tis related to child pays for parent
10:16 < glozow> hint: https://github.com/bitcoin/bitcoin/blob/5ab8cb23e46152957d80f777310ec2493427a19e/doc/policy/packages.md#definitions
10:17 < stickies-v> all transactions in the package except for the last one need to be used as inputs for the last one (i.e. the child), and all the unconfirmed inputs of the child need to be provided in the package
10:17 < larryruane> to answer that, i'm looking at the definition of `IsChildWithParents()` and there's some fancy c++ `std` stuff going on there! I'm not quite familiar with it
10:17 < glozow> ah, that function wouldn't provide the full answer
10:17 < glozow> stickies-v: bingo
10:18 < glozow> does that definition make sense to everybody?
10:18 < Kaizen_Kintsugi> i read here that this is for transaction batching
10:19 < glozow> batching?
10:19 < Kaizen_Kintsugi> like sending a buch of transactions at once
10:19 < Kaizen_Kintsugi> and then if you need to do a fee bump, this will allow to fee bump on the child transaction only
10:20 < Kaizen_Kintsugi> so the parents don't get left dangling
10:20 < larryruane> "stickies-v bingo" ... is this recursive? 
10:21 < stickies-v> larryruane no it's not recursive, but it's using lambda functions which has a bit of a different syntax
10:21 < stickies-v> larryruane see https://docs.microsoft.com/en-us/cpp/cpp/lambda-expressions-in-cpp?view=msvc-160 for some examples
10:21 -!- mariona55 [~mariona@93.176.128.251] has joined #bitcoin-core-pr-reviews
10:21 < glozow> A child-with-unconfirmed-parents package is a topologically sorted package that consists of exactly one child and all of its unconfirmed parents (no other transactions may be present). The last transaction in the package is the child; each of its inputs must refer to a UTXO in the current chain tip or some preceding transaction in the package.
10:22 < Kaizen_Kintsugi> okay, I can picture that in my head
10:22 < larryruane> By recursive I meant, like D has 3 inputs, which refer to outputs of A,B,C (all 4 in the mempool) ... if A has an input that refers to an X output, (X also in mempool), then can (should) X be in the package? 
10:22 -!- mariona55 is now known as mariona
10:23 < davidbak> (transitive)
10:23 < larryruane> i.e. a tree, whose "root" is the last tx in the package? (t-sorted)
10:23 < larryruane> davidbak: +1
10:23 < MarcoFalke> larryruane: That wouldn't be a package, IIUC. You'd have a package of [X,A]
10:24 < glozow> larryruane: ah i see what you're saying. no, we're only considering 2-generation packages
10:24 < larryruane> glozow: +1 thanks
10:24 < glozow> but in terms of the looser definition of a package, yes, any tree like that is a package
10:24 < davidbak> by "looser" you mean the long-term vision of which this is a stepping-stone towards?
10:25 < glozow> uhhh i don't think we are necessarily stepping towards allowing all packages
10:27 < glozow> How does this PR implement checking that a package is child-with-unconfirmed-parents? Why do we add the child’s inputs to coins_to_uncache beforehand?
10:27 -!- azorcode [~azorcode@186-88-138-42.genericrev.cantv.net] has joined #bitcoin-core-pr-reviews
10:27 < glozow> hint: implemented here https://github.com/bitcoin-core-review-club/bitcoin/blob/306a0f6f14972d73281a022d67775a5485d563c7/src/validation.cpp#L1207-L1250
10:28 < Kaizen_Kintsugi> is it to make sure you get the child inputs from the utxo set?
10:28 < davidbak> ok ... is that because the use-case is mainly CPFP at this time and this topology (multi-parent 1-child) is sufficient for that?
10:29 -!- Dulce [~Dulce@172.92.166.62] has joined #bitcoin-core-pr-reviews
10:30 < glozow> davidbak: yes, and because arbitrary packages = boundless complexity. we'd either be allowing DoS attacks or using imperfect heuristics that could open up pinning vectors
10:30 < davidbak> tnx
10:31 < stickies-v> glozow we check that each input tx is either in parent_txids or in the UTXO set with m_view.HaveCoin. We keep the coins_to_uncache vector to remove newly added outputs from the cache if the package gets rejected
10:31 < glozow> stickies-v: exactly!
10:32 < Kaizen_Kintsugi> oh cool
10:32 < glozow> we might be pulling coins from disk into our UTXO set cache. there's no reason to keep them cached
10:32 < stickies-v> glozow I was confused as to why we check if it's in the cache in the first place though - is this purely for performance reasons? 
10:32 < stickies-v> okay yeah your previous message seems to indicate that
10:33 < glozow> stickies-v: good question. yes it's basically performance. our cache is of limited size, and we want it to store coins that we might need again in the near future
10:33 < glozow> for example, if we add a transaction to our mempool, it's a good idea to keep it in the cache because we'll probably look up those inputs when we see the tx in a block later
10:34 < larryruane> and there's no way to access the coins details _without_ bringing it into cache
10:34 < stickies-v> alright yeah that makes sense, thanks!
10:35 < glozow> remember that any attacker on P2P can send us a package (and we'll look up all the inputs to validate the transactions), so they could be trying to make us cache thrash
10:35 < glozow> larryruane: right exactly
10:35 < Kaizen_Kintsugi> these damn attackers are always up to no good
10:35 < larryruane> do we do something like, increase the peer's ban score if that happens a lot?
10:35 -!- azorcode [~azorcode@186-88-138-42.genericrev.cantv.net] has quit [Quit: Ping timeout (120 seconds)]
10:36 -!- gene [~gene@gateway/tor-sasl/gene] has joined #bitcoin-core-pr-reviews
10:36 < stickies-v> it looks so easy to miss those vulnerabilities if you're not very familiar with the codebase at large, damn
10:36 < Kaizen_Kintsugi> for real
10:37 < jnewbery> larryruane: disconnecting/banning is dangerous. If one of those damn attackers is able to make you send a transaction to your peer that would cause them to disconnect you, then they could use that to isolate you, or do it across the network to cause a network split
10:37 < larryruane> (for the newer people here, if a peer misbehaves in some possibly-innocuous way, we bump up its "ban score" which means we don't ban it yet, but if the ban score gets too high, then we do ban it (disconnect, i think?))
10:37 < glozow> larryruane: they can just disconnect and reconnect as a new peer so assigning a ban score wouldn't do much
10:37 < larryruane> jnewbery: +1 thanks
10:37 < Kaizen_Kintsugi> man these are interesting subtle problems
10:38 < glozow> Next question: why might we distinguish between `PCKG_BAD` and `PCKG_POLICY`? Within this PR, do we do anything differently based on the result type?
10:38 -!- azorcode [~azorcode@186-88-138-42.genericrev.cantv.net] has joined #bitcoin-core-pr-reviews
10:38 < jnewbery> if we gave a peer misbehavior points for causing us to fetch UTXOs from disk for a transaction that turned out to fail because of policy, then that could open up those eclipse/netsplit vectors.
10:39 < Kaizen_Kintsugi> Bad packages seem to return a invalid package_state
10:39 < davidbak> jnewbery: what does "eclipse" mean here?
10:39 < glozow> there's a discussion here: https://github.com/bitcoin/bitcoin/pull/22674/files#r732922423
10:40 < jnewbery> davidbak: https://bitcoinops.org/en/topics/eclipse-attacks/
10:40 < stickies-v> glozow you want to differentiate between two categories of package validation failure, one is network consensus failure (PCKG_BAD) and should not be accepted by anyone - so maybe bad faith? The other is local policy failure (PCKG_POLICY), i.e. the rules each node can tweak, which don't necessarily indicate malicious intent?
10:40 < glozow> stickies-v: yes! :))))
10:41 < jnewbery> stickies-v: very close, except that PCKG_BAD isn't necessarily a consensus failure
10:41 < jnewbery> it's more a violation of a (future) p2p protocol rule
10:41 < glozow> network protocol or consensus 
10:41 < jnewbery> that a peer should only send us a package that is one-child-all-unconfirmed-parents
10:42 < glozow> consensus would be `PCKG_TX_CONSENSUS`
10:42 < stickies-v> okay yep makes sense - it's easy to confuse terms sometimes, thanks!
10:43 -!- lucasdcf [~lucasdcf@2804:431:c7d9:3516:16c0:9872:61fe:b8b5] has quit [Ping timeout: 256 seconds]
10:43 < Kaizen_Kintsugi> so a package has to be one child and multiple parents all unconfirmed. If it is anything else it is rejected and coins are removed from the utxo set
10:43 < stickies-v> well and I suppose my labeling of malicious intent is a bit too harsh as well - could just be that a peer has a buggy implementation of what's an allowed package, for example
10:43 < jnewbery> I think in this PR, we don't do anything differently based on whether the package fails for `PCKG_BAD` or `PCKG_POLICY`, but we could do in future when we update p2p to be able to relay packages
10:44 < larryruane> Kaizen_Kintsugi: from the cache, not the UTXO set
10:44 < Kaizen_Kintsugi> ah ty
10:44 < glozow> jnewbery: correct
10:44 < glozow> in the future we would punish/disconnect for `PCKG_BAD` and `PCKG_TX_CONSENSUS`
10:44 < jnewbery> stickies-v: right, it's sometimes not possible to tell the two apart, and our response to either should be the same in any case
10:44 < larryruane> I noticed there are no functional (python) tests, is that because we don't have the P2P messages for packages yet? The unit test is nice
10:45 < glozow> because there is no way to hit this code path through the functional tests :P
10:45 -!- lucasdcf [~lucasdcf@2804:431:c7d9:3516:16c0:9872:61fe:b8b5] has joined #bitcoin-core-pr-reviews
10:46 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has quit [Quit: https://quassel-irc.org - Chat comfortably. Anywhere.]
10:46 < jnewbery> larryruane: there's no RPC for package submission either. This PR is only making the internal changes to the mempool submission logic (and it's already a large PR!)
10:46 < glozow> if you want to look ahead to future commits in #22290, i've added the submitrawpackage RPC and then functional tests
10:46 < davidbak> there was a way in a previous PR to "dry run" packages (at that time) - could that have been done in this case?  or ... is it or is it not a B.C. dev policy to modify RPC handling to account for new-but-incompletely exposed features?
10:47 < davidbak> (or maybe I got confused between future and past PRs, sorry)
10:47 < glozow> davidbak: yes that's correct, you're referring to #20833
10:47 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews
10:48 < glozow> that added about 1/2 of the validation logic here
10:48 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has quit [Client Quit]
10:48 < glozow> and lots of testing
10:48 < glozow> but we wouldn't be able to test package submission through testmempoolaccept, for instance
10:49 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews
10:49 < glozow> Okay next question: let's look at this line of code which is taking the virtual size of a transaction to be returned in testmempoolaccept https://github.com/bitcoin-core-review-club/bitcoin/commit/78d3665a6d9663cdad188243c6be7e8e9e9ece4e#diff-a58e7bb9d9a8a0287c0b7281d99da4e79b6f8c2a5780c24c6d76c14212c48640L981
10:50 < glozow> In what scenarios could the virtual sizes obtained from GetVirtualTransactionSize() here and the MempoolAcceptResult be different?
10:50 < glozow> (Hint: there are at least 2 ways)
10:50 < davidbak> so in the implementation of a feature over multiple PRs (what the Agile people would call stories in an epic) it is ok (from the POV of the maintainers) to have individual PRs that do not include their own testing as long as you get it in a subsequent PR? (trying to learn the dev philosophy here as well as this individual PR)
10:51 < glozow> davidbak: the code added in this PR _is_ tested, in the unit tests
10:51 < davidbak> i get that, yes
10:52 < glozow> i would say that it's more appropriate to test internal mempool validation from a unit test
10:52 < jnewbery> There's a distinction between unit tests, which are testing the logic in individual units of code (in this case the mempool submission/acceptance logic), and functional tests, which test end-to-end user functionality.
10:52 < jnewbery> glozow: +1, it's much more appropriate to use unit tests here
10:53 < Kaizen_Kintsugi> glozow: if there was a fee bump? shit I'm grasping at straws here
10:53 < glozow> in this case there is no change in end-to-end user functionality
10:53 < davidbak> ok, i do know the difference, and I see that POV fine
10:53 < Kaizen_Kintsugi> or maybe a invalid package is submitted?
10:53 < larryruane> just a wild guess, maybe if the witness is different?
10:53 < glozow> larryruane: bingo!
10:53 < larryruane> haha just lucky :)
10:53 < jnewbery> in general, I think this project relies too much on the python functional tests, because they're easy to write and our code is not structured in a way that makes it easy to isolate and test individual components
10:54 < glozow> that's one possibility. right now we don't have witness replacement ;) so the RPC will just return the transaction that's already in the mempool. if they have different witness sizes then this would be inaccurate!
10:54 < Kaizen_Kintsugi> oh shit, i was thinking it was related to the 'weight' from segwit
10:55 < larryruane> jnewbery: +1 when a python functional test fails, it can be hell to figure out why (so much code is looped in)
10:55 < glozow> there's another way the virtual size could be different, and it's if the transaction has a toooon of signatures so its "sigop weight" is higher than its BIP141 weight
10:55 < jnewbery> larryruane: exactly right. They're _slow_ as well, since they're spinning up one or more full bitcoind nodes. They run in seconds instead of milliseconds
10:56 < davidbak> jnewbery: i appreciate that comment, thanks; a different POV, by the way, is that if this PR already has PCKG_BAD then that _is_ an end-to-end user functionality; won't be _exposed_ until the P2P stuff is in later, but if not tested here it adds to the test burden _then_.  But it's just an argument, I accept your conclusion that unit tests are appropriate here.
10:57 < stickies-v> glozow but it looks like both GetVirtualTransactionSize and MempoolAcceptResult consider the sigop cost? so how would they differ?
10:57 < glozow> `GetVirtualTransactionSize` with just a tx as argument doesn't use sigop cost AFAIK
10:57 < larryruane> glozow: hope this isn't a sidetrack (ignore if you'd like) but is sigop weight a little like gas in ethereum, so that there's some consideration of the CPU time needed to evaluate scripts? I'm really not familar with sigop weight
10:58 < stickies-v> I looked at policy.cpp and it looks like it does? https://github.com/bitcoin/bitcoin/blob/4dbba3bac70f78e764910f357c875c09569a8fc4/src/policy/policy.cpp#L285
10:58 < glozow> yeah it's just a wrapper for `GetVirtualTransactionSize(tx, 0, 0)`
10:59 < glozow> yes, that's the one called within `PreChecks`
10:59 < glozow> the one being called in the RPC code is the one with 1 argument: https://github.com/bitcoin/bitcoin/blob/4dbba3bac70f78e764910f357c875c09569a8fc4/src/policy/policy.h#L126-L129
10:59 < stickies-v> ah, dang! shouldn't just have done a simple github search, my bad
11:00 < glozow> larryruane: I agree that limiting sigops is way of limiting CPU expenditure
11:00 < glozow> sigop "weight" is just a heuristic used in the mempool
11:01 < glozow> Miners seek to maximize the total transaction fees while ensuring that their blocks are within consensus-enforced weight and sigops limits. To simplify this 2-dimensional knapsack problem, in the mempool, virtual size of a transaction is calculated as the maximum between its BIP141 serialized size and its “sigop weight”.
11:01 < jnewbery> larryruane: I think this is where the sigops limit was first introduced: https://github.com/bitcoin/bitcoin/commit/8c9479c6bbbc38b897dc97de9d04e4d5a5a36730#diff-608d8de3fba954c50110b6d7386988f27295de845e9d7174e40095ba5efcf1bbR1425-R1427
11:01 < larryruane> okay I see, not consensus (like ETH gas)
11:01 < glozow> #endmeeting
11:02 < jnewbery> larryruane: it is a consensus rule
11:02 < jnewbery> thanks glozow. Great meeting!
11:02 < glozow> v sad that we didn't get to all the questions, but thank you all for the engaging conversation :)
11:02 < gene> more optimized for space on chain, right?
11:02 < stickies-v> larryruane consensus limits the total amount of sigops in a block though, so miners need to be careful not to include too many sigops in a small/cheap transaction
11:02 < davidbak> big important PR, only 1hr, that's to be expected!
11:03 < stickies-v> hence why it's being bundled in the same heuristic to, like glozow said, make constructing the optimal block more straightforward computationally
11:03 < Kaizen_Kintsugi> man this was so awesome
11:03 < Kaizen_Kintsugi> looking forward to next week
11:03 < Kaizen_Kintsugi> thx!
11:04 < stickies-v> agreed, very interesting PR again, thanks for the extensive prep glozow - the questions were really on point to dig through the code!
11:05 < davidbak> not just extensive prep for this meeting, but extensive prep for the PR: the additional written documentation _with excellent drawings_ is a great example of a way to get a PR accepted (to my mind anyway, correct me if I'm wrong)
11:07 < hernanmarino> thanks glozow, great meeting and interesting topic 
11:08 < stickies-v> +1 davidbak , amazing documentation indeed
11:21 -!- stickies-v [~stickies-@84-45-243-170.static.enta.net] has quit [Quit: Computer going to sleep.]
11:21 -!- schlamarcel [~schlamarc@2a02:8109:b540:5aa1::c8] has quit [Quit: Client closed]
11:21 -!- azorcode [~azorcode@186-88-138-42.genericrev.cantv.net] has quit [Ping timeout: 256 seconds]
11:24 -!- stickies-v [~stickies-@84-45-243-170.static.enta.net] has joined #bitcoin-core-pr-reviews
11:28 -!- stickies-v [~stickies-@84-45-243-170.static.enta.net] has quit [Client Quit]
11:35 -!- stickies-v [~stickies-@84-45-243-170.static.enta.net] has joined #bitcoin-core-pr-reviews
11:43 -!- stickies-v [~stickies-@84-45-243-170.static.enta.net] has quit [Ping timeout: 246 seconds]
11:56 -!- Kaizen_Kintsugi [~Kaizen_Ki@node-1w7jr9yi65te7aswxjyf8efmc.ipv6.telus.net] has quit [Read error: Connection reset by peer]
11:57 -!- Kaizen_Kintsugi [~Kaizen_Ki@node-1w7jr9yi65te7aswxjyf8efmc.ipv6.telus.net] has joined #bitcoin-core-pr-reviews
11:59 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
12:02 -!- mariona [~mariona@93.176.128.251] has quit [Quit: Client closed]
12:03 -!- mariona [~mariona@93.176.128.251] has joined #bitcoin-core-pr-reviews
12:10 -!- lucasdcf [~lucasdcf@2804:431:c7d9:3516:16c0:9872:61fe:b8b5] has quit [Ping timeout: 256 seconds]
12:14 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has quit [Quit: Connection closed for inactivity]
12:27 -!- jrawsthorne [~jrawsthor@static.235.41.217.95.clients.your-server.de] has quit [Quit: ZNC 1.8.1 - https://znc.in]
12:27 -!- jrawsthorne [~jrawsthor@static.235.41.217.95.clients.your-server.de] has joined #bitcoin-core-pr-reviews
12:35 -!- Dulce [~Dulce@172.92.166.62] has quit [Quit: Client closed]
12:58 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:a443:99fa:7edf:26be] has quit [Quit: Client closed]
13:04 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has quit [Ping timeout: 268 seconds]
13:11 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has joined #bitcoin-core-pr-reviews
13:15 -!- mariona [~mariona@93.176.128.251] has quit [Quit: Client closed]
14:29 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:a443:99fa:7edf:26be] has joined #bitcoin-core-pr-reviews
14:32 < larryruane> I didn't have a chance to ask, what's the idea of using `SaltedTxidHasher` here? Why does it need to be salted (or does it)? https://github.com/bitcoin/bitcoin/pull/22674/files#diff-883e0d00cab45c5643bd70e35b318436f43b50a80b17aa2eb439fd4fa57ca8f7R73
15:05 -!- yonson [~yonson@2600:8801:d900:7bb:1e69:7aff:fea2:4e85] has quit [Remote host closed the connection]
15:05 -!- yonson [~yonson@2600:8801:d900:7bb:1e69:7aff:fea2:4e85] has joined #bitcoin-core-pr-reviews
15:39 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:a443:99fa:7edf:26be] has quit [Quit: Client closed]
15:50 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has quit [Quit: Connection closed for inactivity]
16:02 -!- grettke [~grettke@cpe-65-29-228-30.wi.res.rr.com] has quit [Quit: Textual IRC Client: www.textualapp.com]
16:06 -!- grettke [~grettke@cpe-65-29-228-30.wi.res.rr.com] has joined #bitcoin-core-pr-reviews
17:05 -!- gene [~gene@gateway/tor-sasl/gene] has quit [Remote host closed the connection]
20:46 -!- Hernan [~hernanmar@OL121-235.fibertel.com.ar] has joined #bitcoin-core-pr-reviews
20:48 -!- hernanmarino [~hernanmar@OL121-235.fibertel.com.ar] has quit [Ping timeout: 268 seconds]
20:48 -!- Hernan_ [~hernanmar@OL121-235.fibertel.com.ar] has joined #bitcoin-core-pr-reviews
20:51 -!- hernanmarino [~hernanmar@OL121-235.fibertel.com.ar] has joined #bitcoin-core-pr-reviews
20:51 -!- Hernan [~hernanmar@OL121-235.fibertel.com.ar] has quit [Ping timeout: 260 seconds]
20:52 -!- Hernan [~hernanmar@OL121-235.fibertel.com.ar] has joined #bitcoin-core-pr-reviews
20:53 -!- Hernan_ [~hernanmar@OL121-235.fibertel.com.ar] has quit [Ping timeout: 245 seconds]
20:56 -!- hernanmarino [~hernanmar@OL121-235.fibertel.com.ar] has quit [Ping timeout: 268 seconds]
21:17 -!- Hernan [~hernanmar@OL121-235.fibertel.com.ar] has quit [Quit: Leaving]
--- Log closed Thu Oct 28 00:00:16 2021