--- Log opened Wed Nov 17 00:00:35 2021
00:01 -!- b10c [uid500648@ilkley.irccloud.com] has joined #bitcoin-core-pr-reviews
00:14 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
00:18 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Ping timeout: 265 seconds]
02:03 -!- kexkey [~kexkey@static-198-54-132-167.cust.tzulo.com] has quit [Ping timeout: 265 seconds]
02:05 -!- kexkey [~kexkey@static-198-54-132-103.cust.tzulo.com] has joined #bitcoin-core-pr-reviews
02:13 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
02:17 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Ping timeout: 265 seconds]
03:10 -!- b10c [uid500648@ilkley.irccloud.com] has quit [Quit: Connection closed for inactivity]
03:24 -!- amiti [sid373138@lymington.irccloud.com] has quit [Read error: Connection reset by peer]
03:24 -!- amiti [sid373138@lymington.irccloud.com] has joined #bitcoin-core-pr-reviews
03:25 -!- lsilva_ [sid489830@helmsley.irccloud.com] has quit [Ping timeout: 256 seconds]
03:25 -!- stick [sid403625@user/prusnak] has quit [Ping timeout: 264 seconds]
03:28 -!- lsilva_ [sid489830@helmsley.irccloud.com] has joined #bitcoin-core-pr-reviews
03:28 -!- stick [sid403625@user/prusnak] has joined #bitcoin-core-pr-reviews
03:32 -!- raj [~raj@2602:ffb6:4:e396:f816:3eff:fe47:ca2a] has quit [Killed (NickServ (GHOST command used by raj_!uid72176@user/raj))]
03:32 -!- raj [~raj@2602:ffb6:4:e396:f816:3eff:fe47:ca2a] has joined #bitcoin-core-pr-reviews
03:33 -!- raj [~raj@2602:ffb6:4:e396:f816:3eff:fe47:ca2a] has quit [Killed (NickServ (GHOST command used by raj_!uid72176@user/raj))]
03:33 -!- raj [~raj@2602:ffb6:4:e396:f816:3eff:fe47:ca2a] has joined #bitcoin-core-pr-reviews
03:34 -!- raj [~raj@2602:ffb6:4:e396:f816:3eff:fe47:ca2a] has quit [Killed (NickServ (GHOST command used by raj_!uid72176@user/raj))]
03:34 -!- raj [~raj@2602:ffb6:4:e396:f816:3eff:fe47:ca2a] has joined #bitcoin-core-pr-reviews
03:34 -!- raj [~raj@2602:ffb6:4:e396:f816:3eff:fe47:ca2a] has quit [Killed (NickServ (GHOST command used by raj_!uid72176@user/raj))]
03:34 -!- raj_ [~raj@2602:ffb6:4:e396:f816:3eff:fe47:ca2a] has joined #bitcoin-core-pr-reviews
03:36 -!- lightlike [sid521075@user/lightlike] has quit [Ping timeout: 256 seconds]
03:41 -!- lightlike [sid521075@user/lightlike] has joined #bitcoin-core-pr-reviews
04:05 -!- Weekdayz [~Weekdayz@102.89.2.227] has joined #bitcoin-core-pr-reviews
05:09 -!- kexkey_ [~kexkey@static-198-54-132-135.cust.tzulo.com] has joined #bitcoin-core-pr-reviews
05:12 -!- kexkey [~kexkey@static-198-54-132-103.cust.tzulo.com] has quit [Ping timeout: 256 seconds]
05:45 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
05:50 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Ping timeout: 256 seconds]
06:08 -!- b10c [uid500648@ilkley.irccloud.com] has joined #bitcoin-core-pr-reviews
07:02 < pinheadmz_> why does bitcoin core release soft forks in minor versions? for example, taproot code was released in 0.21.1 five months after 0.21.0 -- is that simply because soft forks are not breaking changes? so its semantic?
07:04 < pinheadmz_> oh ! i found the answer https://bitcoincore.org/en/lifecycle/#schedule
07:06 < sipa> yeah, they're treated as bug fixes, because they happen at least partially outside of bitcoin core's control, and afterwards are incorporated in all sufficiently maintained branches
07:07 < pinheadmz_> afterwards you mean after activation / lock in ?
07:07 < sipa> for the last few we've also followed an approach where the actual softfork code is included in an 0.x.0 so it can be tested in test networks etc, and then have the activation in 0.x.1
07:25 -!- willcl_ark [~quassel@user/willcl-ark/x-8282106] has quit [Ping timeout: 268 seconds]
07:27 -!- willcl_ark [~quassel@user/willcl-ark/x-8282106] has joined #bitcoin-core-pr-reviews
07:33 -!- zcw [~zcw@209.203.21.89] has joined #bitcoin-core-pr-reviews
08:42 -!- asfalto [~asfalto@host-2-103-190-113.as13285.net] has joined #bitcoin-core-pr-reviews
08:45 -!- asfalto [~asfalto@host-2-103-190-113.as13285.net] has quit [Client Quit]
08:47 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
08:48 < jnewbery> hi folks. We start in just over 10 minutes.
08:50 -!- sekulicd [~sekulicd@79.140.150.38] has joined #bitcoin-core-pr-reviews
08:51 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Ping timeout: 256 seconds]
08:52 -!- gene [~gene@gateway/tor-sasl/gene] has joined #bitcoin-core-pr-reviews
08:57 -!- maxe [~maxe@host-2-103-190-113.as13285.net] has joined #bitcoin-core-pr-reviews
08:59 -!- Weekdayz [~Weekdayz@102.89.2.227] has quit [Quit: Connection closed]
08:59 -!- effexzi [uid474242@ilkley.irccloud.com] has joined #bitcoin-core-pr-reviews
08:59 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has joined #bitcoin-core-pr-reviews
09:00 < jnewbery> #startmeeting
09:00 < stickies-v> hi everyone
09:00 < raj_> hello..
09:00 < glozow> hi
09:00 < b10c> hi
09:00 < svav> Hi All
09:00 < neha> hi
09:00 < jnewbery> hi folks. Welcome to PR Review Club. Feel free to say hi to let people know you're here
09:00 < maxe> hi
09:01 < jnewbery> Is anyone here for the first time?
09:01 < pg156> hi
09:01 < michaelfolkson> hi
09:01 < maxe> I'm first time
09:01 < gene> hi
09:01 -!- Weekdayz [~Weekdayz@102.89.2.227] has joined #bitcoin-core-pr-reviews
09:01 < schmidty> hi
09:01 < jnewbery> maxe: you're very welcome! Feel free to ask questions at any point if anything is unclear. We're all here to learn together
09:01 < effexzi> Hi
09:02 < jnewbery> Notes and questions are in the normal place: https://bitcoincore.reviews/23319
09:02 < maxe> jnewbery: thanks for the warm welcome
09:02 < jnewbery> I’ll use those questions to guide the conversation, but feel free to jump in at any point if you have questions or comments.
09:02 -!- danp [~danp@174-21-103-147.tukw.qwest.net] has joined #bitcoin-core-pr-reviews
09:02 < jnewbery> Who had a chance to review the PR and notes/questions this week? (y/n)
09:02 < stickies-v> y
09:02 < pg156> y
09:02 < maxe> y
09:03 < larryruane> hi y (mostly)
09:03 < raj_> mostly y
09:03 < glozow> n
09:03 < neha> n
09:03 < jnewbery> Let's get into the questions. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?
09:03 < pg156> I read the notes and questions and the PR, but I don't have enough information to give a concept ACK, because I don't understand how the fee data will exactly help the user. What are the use cases?
09:04 < stickies-v> Approach ACK, waiting for a bug to be fixed before tACK
09:04 -!- Azorcode [~Azorcode@186-90-4-130.genericrev.cantv.net] has joined #bitcoin-core-pr-reviews
09:04 < raj_> concept ACK, currently theres a test failure..
09:05 < maxe> I will say Concept ACK but experience with codebase none existent
09:05 < larryruane> concept, approach, light-tested ACK
09:05 -!- zcw [~zcw@209.203.21.89] has quit [Quit: Connection closed]
09:05 < jnewbery> pg156: it looks like there's not much motivation for this change given in the PR or associated issue. Perhaps someone else here can give some use-cases?
09:06 -!- sekulicd [~sekulicd@79.140.150.38] has quit [Quit: Connection closed]
09:06 < maxe> I noticed this in the notes: "Initial concept ACK. This is really helpful for wallets trying to sync with core rpc out there, and it will reduce get calls by half for input fetching. Thanks for working on this."
09:06 < larryruane> except for me, `test/functional/rpc_rawtransaction.py` fails
09:07 < raj_> jnewbery, I find the input details reporting very helpful. I am working on a wallet that syncs from bitcoin core with RPC, to get the input details we had to make many rpc get queries. This will reduce those get calls by a lot.. 
09:07 < jnewbery> raj_: nice
09:07 < michaelfolkson> raj_: But you could process the fee calculation externally to the Core RPC right?
09:08 < jnewbery> I think fee information is probably generally useful for people building block explorers or other similar tools to read historic data
09:08 < michaelfolkson> I agree it seems slightly more convenient (at least)
09:08 < raj_> michaelfolkson, yes, but not without getting the input details. And yes, once we get the inputs, getting fee is mostly trivial. So not very sure on the fee part.
09:09 < michaelfolkson> Right
09:09 < jnewbery> Next question: This PR changes the second argument of getrawtransaction from verbose (a bool) to verbosity (an int). Is this ok? Does it break backwards compatibility?
09:09 -!- Azorcode [~Azorcode@186-90-4-130.genericrev.cantv.net] has quit [Quit: Connection closed]
09:09 < pg156> It maintains backwards compatibility by this line:
09:09 < pg156> `verbosity = request.params[1].get_bool() ? 1 : 0;`
09:09 < pg156> So if the argument is bool, it will be converted to 0 or 1.
09:09 < lsilva_> Does not break backwards compatibility. If the user passes a boolean parameter, the code will still handle it (src / rpc / rawtransaction.cpp: 201-205).
09:10 < pg156> But where is "request" created? Is it in this line? https://github.com/bitcoin-core-review-club/bitcoin/blob/f30f007c829547e44cac4214c04a0fa7d0ddc3c2/src/rpc/rawtransaction.cpp#L158
09:10 < maxe> a comment in the release notes suggests the author considered backwards compatibility
09:10 < larryruane> maybe very slightly non-backward compatible, because before this PR, specifying a 2 gives the old verbose but now with this PR, a more verbose output ... so that's a change in behavior (the the same input), but that's undocumented usage, so it's okay
09:11 -!- Azorcode [~Azorcode@186-90-4-130.genericrev.cantv.net] has joined #bitcoin-core-pr-reviews
09:11 < jnewbery> larryruane: very nice observation!
09:11 < sipa> larryruane: (i haven't reviewed the PR in detail) even if you do that, does it actually change any output fields, or just add more?
09:12 < larryruane> oh but wait, correcting myself, all this does is add *new* fields to the output, so anyone looking at the existing fields will find them (with the same meanings)
09:12 < larryruane> sipa: yes, you beat me to that comment :)
09:13 < neha> thanks for that clarification! so iiuc clients who might have been passing a 2 or 3 for a bool (which is weird, but ok) should be fine? does it break anything to have extra fields?
09:13 < michaelfolkson> Nice comment by stickies-v: "Did some testing and it looks like the RPC is failing for coinbase transactions when verbosity==2. In this case we should probably fallback to verbosity==1 behaviour instead?"
09:13 < larryruane> question: would it be okay to add a new verbosity level (2) and have it cause the removal of some fields? (or changing their meaning?)
09:13 < jnewbery> sipa larryruane: right, I think it just adds new fields to the object being returned. That should be fine - clients should tolerate receiving fields that they don't know the meaning off, since we often upgrade RPCs by adding new fields
09:13 < larryruane> (not that we would ever want to do that, but just curious)
09:14 < michaelfolkson> That's a backward compatibility issue (at least with the current state of the PR). I'm assuming it should never fail (once PR issue is sorted)
09:14 < larryruane> michaelfolkson: can you explain what "PR issue is sorted" means for us newbies?
09:14 -!- grettke [~grettke@cpe-65-29-228-30.wi.res.rr.com] has joined #bitcoin-core-pr-reviews
09:14 < sipa> larryruane: that'd be an incomaptible change, and we usually follow RPC deprecation guidelines for that
09:15 < larryruane> sipa: +1 thanks
09:15 < sipa> (first add a -deprecatedrpc=bla argument to have it retain the field, later drop it)
09:15 < stickies-v> would we be breaching any API guarantees by simply adding the new `fee` and `prevout` in the verbose output, without adding the new verbosity==2 option? Since we're just adding new fields?
09:15 < glozow> neha: any software that's passing getrawtransaction(2)["somefield"] will still work. if there is software asserting (getrawtransaction(2) == someobject) then it fails, but that just means they need to change the 2 to a true
09:15 < glozow> (afaiu)
09:15 < larryruane> stickies-v: I think that would be okay, BUT, performance would be worse (and possibly of no benefit)?
09:16 < jnewbery> larryruane: that *could* be a break in user behaviour, *if* the users is calling the RPC with verbose={some value greater than 1} *and* they were expecting that field to always be there. However, that's a pretty pathological edgecase, since users shouldn't really have been setting verbose={some value greater than 1}
09:16 < neha> glozow: thanks! my question is does anyone do the latter, or I guess more like what is the API contract around that? Is it considered backwards-incompatible for that to change?
09:16 < michaelfolkson> larryruane: Ha, I'm just referring to the RPC failing (stickies-v comment)
09:16 < merkle_noob[m]> Hi everyone(Late again :(   )
09:16 < michaelfolkson> https://github.com/bitcoin/bitcoin/pull/23319#pullrequestreview-806345918
09:17 < glozow> neha: i think i'd consider that backwards compatible. i can imagine the latter being used in a test maybe?
09:17 < neha> based on jnewbery's "clients should tolerate receiving fields that they don't know the meaning off, since we often upgrade RPCs by adding new fields" i agree
09:18 < jnewbery> *meaning of. oops
09:18 < stickies-v> larryruane ah yes true, there is a performance impact, thanks
09:18 < neha> just curious what the API contract is or if it's documented anywhere
09:18 -!- danp [~danp@174-21-103-147.tukw.qwest.net] has quit [Quit: Connection closed]
09:18 < jnewbery> I think really this RPC shouldn't have tolerated receiving a verbose={value greater than 1} previously
09:19 < larryruane> jnewbery: ah but it does currently tolerate verbose=3
09:19 < larryruane> maybe should not?
09:19 < pg156> larryruane: +1. I saw that in code too.
09:19 < glozow> does our RPC argument handling, in general, allow conversions of non-bools to bools?
09:19 < larryruane> it acts like verbose=2
09:20 < lsilva_> I agree.  If verbosity is not between 0 and 2, it should return an error.
09:20 < jnewbery> larryruane: yes it does, but I think it shouldn't!
09:20 < jnewbery> Next question: Why is there a new entry added to vRPCConvertParams? What is vRPCConvertParams used for?
09:21 < pg156> So the second argument can be called with the new named argument "verbosity".
09:21 < lsilva_> vRPCConvertParams is a array of CRPCConvertParam. Its purpose is to convert non-string RPC argument to JSON. It is used by bitcoin-cli when preparing the request in DefaultRequestHandler::PrepareRequest. The methods used to convert values are RPCConvertNamedValues (for named parameters) and RPCConvertValues.
09:22 < larryruane> glozow: I think there needs to be special case code to handle that https://github.com/bitcoin-core-review-club/bitcoin/commit/f30f007c829547e44cac4214c04a0fa7d0ddc3c2#diff-a58e7bb9d9a8a0287c0b7281d99da4e79b6f8c2a5780c24c6d76c14212c48640R172
09:22 < jnewbery> lsilva_: very good!
09:22 < jnewbery> perfect answer
09:23 < larryruane> note that you need to specify `-named` on the `bitcoin-cli` command line, and then ALL arguments need to be named
09:23 < larryruane> but what's nice is then they can be in any order!
09:24 < jnewbery> larryruane: yeah, and you can omit arguments that you don't want to specify (unlike if you're using positional arguments)
09:24 < larryruane> example `src/bitcoin-cli -named getrawtransaction txid=f420bf49b355894783ed5c62bd7dfb23c48aa3eb3b206e094f57bc604506e327 verbosity=2`
09:24 < jnewbery> Next question: The verbosity argument accepts values of 0 (returns the hex-encoded data for the transaction), 1 (returns an object with information about the transaction) or 2 (returns an object with information about the transaction, including fees and prevout information for the inputs). What happens if 3 is passed as the argument? How about -1?
09:25 < stickies-v> Any verbosity value that is a valid integer and not `0` or `1` is treated as if it were `2`.
09:25 < lsilva_> If verbosity is not between 0 and 2, it will be considered as 2.
09:26 < jnewbery> I think so too. Does that seem desirable?
09:27 < pg156> What's the C++ idiomatic way to express a type could be either a bool, or an integer of value 0, 1, 2? An abstract class? I kno in other languages this could be easier. (subtyping?)
09:27 < schmidty> Not great for backward compatibility
09:27 < jnewbery> schmidty: I agree!
09:27 < gene> pg156: std::variant or a union
09:27 < neha> depends on the API contract with users. it sounds like you are not allowed to remove fields, only add them, which means that any update to use other verbosity levels would need to abide by that
09:28 < pg156> gene: thanks
09:28 < glozow> should return an rpc error for a bad argument like -1
09:28 < neha> that seems limiting for values of verbosity < 0
09:28 < lsilva_> I don't think it is a desirable behavior.
09:28 < stickies-v> I generally prefer things to fail explicitly. I don't see any benefit of accepting these other values, except slightly reducing the complexity of the code by avoiding error checking
09:28 < michaelfolkson> stickies-v: Agree
09:29 < sipa> conceptually i agree, but i don't think it actually matters here
09:29 < jnewbery> neha: I think if in some version of the code, calling the RPC with verbosity=n fails, and in the next version of the code, calling the RPC with verbosity=n succeeds, but has some of the fields missing that you'd get with verbosity=n-1, that's fine
09:30 < jnewbery> it's only if calling the RPC in the same way removes fields from one version to the next, we need to be careful
09:31 < jnewbery> ok, next question. The commit introduces local variables blockUndo and block in the getrawtransaction() function. What are they used for? How/where do they get set?
09:32 < lsilva_> They are used to retrieve confirmed transaction from block files. 
09:32 < stickies-v> They contain the unserialized CBlockUndo and CBlock data, which get set by calling the UndoReadFromDisk and ReadBlockFromDisk functions (using out parameters). `blockUndo` is used to calculate the fee more quickly since it already contains the prevouts. `block` is used to find the index of the transaction in the block, since that is necessary to find the correct undo data in `blockUndo`.
09:33 < lsilva_> What if the inputs are in the mempool?
09:34 < lsilva_> Or are the inputs in several different blocks?
09:34 < lsilva_> Or *if the inputs are in several different blocks?
09:35 < jnewbery> stickies-v: yes, I agree
09:36 < stickies-v> lsilva_ it seems like no fee or prevout data will be included in the RPC response then, because the blockindex would be null so it would fail the !blockindex test. Just thinking out loud here, anyone has any other views?
09:37 < jnewbery> lsilva_: good questions. Did you test either of those?
09:37 -!- ccdle12 [~ccdle12@243.222.90.149.rev.vodafone.pt] has joined #bitcoin-core-pr-reviews
09:37 < stickies-v> ^well, pass the `!blockindex` test, so behaviour of verbosity==1 is used
09:38 < pg156> Why do we need both `CBlock` and `CBlockUndo`? Is it possible to have one class `CBlock` (and only block*.dat files without rev*.dat files) handle the block reverting as well?
09:38 < stickies-v> inputs being in different blocks is no issue, that's why the have the undo data - it stores all the prevouts of each transaction in the block in its own data structure so you don't need to look them all up
09:39 < larryruane> question, `blockUndo` and `block` get default-constructed even if `verbosity` is 1, is that a performance concern? (I didn't look at those constructors)
09:39 < jnewbery> I haven't tested the new behaviour for a mempool tx, but I agree with stickies-v that it wouldn't be able to return the fee or prevout data
09:39 < jnewbery> I also agree that it's not a problem for the inputs to be in different blocks for the reason that stickies-v gave
09:39 < lsilva_> But there is only one CBlockUndo blockUndo and one CBlock block. 
09:39 < sipa> pg156: the best argument i think is that they are constructed at a different time, in different order; blocks are received directly from peers in the network; undo data is a side effect of validation, which happens whenever blocks become part of the new longest active chain
09:40 < pg156> sipa: Thanks.
09:40 < sipa> so you'd need to go update files on disk when undo data is produced, reordering/shuffling other data
09:41 < jnewbery> larryruane: The default constructors are very cheap
09:41 < sipa> having it sepatate means both can be essentially append-only data structures
09:41 < larryruane> sipa yes I notice
09:42 < larryruane> sorry, I noticed that the blks files can contain blocks that aren't in the best chain, we don't try to delete them (reuse that space)
09:42 -!- gene [~gene@gateway/tor-sasl/gene] has quit [Remote host closed the connection]
09:42 < sipa> if you care about space, prune
09:42 -!- gene [~gene@gateway/tor-sasl/gene] has joined #bitcoin-core-pr-reviews
09:42 < stickies-v> lsilva_ yep, in this function we're looking at a single transaction, that belongs to a single block. For each block, an equivalent set of undo data is stored on disk. That undo data, for each block, contains all the inputs that were used for each of the transactions in the block. That makes it much easier to roll back a block, because all the prevouts are already stored (duplicated) there.
09:42 < sipa> if you don't, the <1% of stale blocks in the chain aren't going to matter
09:43 < sipa> stickies-v: not for each block!~
09:43 < jnewbery> I think it's slightly bad style that block and blockUndo get updated as side-effects of function calls in the if statement, and then used later
09:44 < lsilva_> stickies-v got it. Thanks.
09:44 < jnewbery> it just makes it a bit more difficult to read
09:44 < stickies-v> sipa oh, could you elaborate on that? when do we not have undo data?
09:44 < larryruane> jnewbery: yes that if statement is somewhat hard to human-parse
09:44 < sipa> stickies-v: for blocks that were never validated
09:44 < larryruane> (like i'm applying demorgan's rule in my head :) )
09:45 < jnewbery> larryruane: 😂
09:45 -!- Guest61 [~Guest61@124.123.169.87] has joined #bitcoin-core-pr-reviews
09:45 < stickies-v> oh okay wasn't aware, thanks! I think that's not a problem in this case since we only use the active chain?
09:45 < jnewbery> stickies-v: the undo data is generated when we connect the block to the tip of the chain (and remove the spent UTXOs from the UTXO set)
09:46 < sipa> stickies-v: that's right, blocks in the active chain are by definition validated
09:46 < sipa> (because the active chain is defined as the most-work valid chain...)
09:47 < larryruane> jnewbery: ah so it's sort of like *moving* the spent TXOs from the UTXO set to the undo "set" (data)
09:47 < jnewbery> if we were writing the ReadBlockFromDisk() and UndoReadFromDisk() interfaces from scratch, we might have them return std::optional<CBlock|CBlockUndo> (as long as that didn't impact performance)
09:48 < jnewbery> Next question: what does this new for loop do? (https://github.com/bitcoin-core-review-club/bitcoin/commit/f30f007c829547e44cac4214c04a0fa7d0ddc3c2#)
09:49 < stickies-v> It finds the position of the transaction in the block, which is needed to look up the correct undo data
09:49 < larryruane> side question, does `opt_tx_position` need to be initialized to `std::nullopt`? Isn't that the default? Or is that just considered good practice?
09:50 < larryruane> stickies-v: +1 and that's a linear search, could be roughly 2k transactions in the block
09:51 < larryruane> could someone explain why, in that loop, `block.vtx.at(i)` instead of `block.vtx[i]`?
09:51 < stickies-v> larryruane I think if you don't initialize it depends on the compiler what the default value is going to be, so hence best to be explicit?
09:51 < gene> larryruane: bounds checking
09:52 < jnewbery> larryruane: if you're concerned about performance, the thing that you should be worried about is deserializing the entire block from disk into CTransaction objects
09:52 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
09:52 < jnewbery> (which happens in ReadBlockFromDisk())
09:52 < larryruane> jnewbery: +1 thanks
09:53 < jnewbery> stickies-v: the default ctor for a std::optional<> is defined by the spec. It creates a std::nullopt : https://en.cppreference.com/w/cpp/utility/optional/optional
09:54 < larryruane> jnewbery: "... deserializing the entire block ..." -- shameless plug, I have an old PR that avoids that most of the time during IBD, speeding it up considerably https://github.com/bitcoin/bitcoin/pull/16981
09:54 < jnewbery> larryruane: shameless plugs are always encouraged :)
09:54 < larryruane> (well, i should say, avoids doing it TWICE)
09:55 < larryruane> (we should really get that merged :) )
09:55 < jnewbery> gene: right, the [] operator on a std::vector doesn't do any bounds checking
09:56 < larryruane> jnewbery: gene: but would you say that since `i` is limited by `size()` it can't be out of bounds?
09:56 < sipa> jnewbery: unless debug mode
09:56 -!- Weekdayz [~Weekdayz@102.89.2.227] has quit [Ping timeout: 268 seconds]
09:57 < jnewbery> larryruane: yes, I agree that bounds checking is not required here since it's done by the for loop
09:57 < stickies-v> larryruane that looks interesting, I'll take a loot at #16981 as my next review
09:57 < gene> larryruane: was thinking the same, unnecessary overhead
09:59 < jnewbery> I'd really like it if there was a C++ equivalent to Python's enumerate: https://docs.python.org/3/library/functions.html#enumerate
09:59 < stickies-v> jnewbery oh okay makes sense, so in this case it would be better to omit the initialization and just have `std::optional<size_t> opt_tx_position;` for improved readability without any downsides? 
09:59 < jnewbery> which would return a std::pair of the counter and object
09:59 < jnewbery> unfortuantely, that's not something in the c++ language or standard library
10:00 -!- ragu [~ragu@pool-74-108-108-236.nycmny.fios.verizon.net] has joined #bitcoin-core-pr-reviews
10:00 < jnewbery> stickies-v: yes, I think it's fine to use the default initialization there
10:00 < jnewbery> ok, that's time
10:00 < jnewbery> #endmeeting
10:01 < sipa> jnewbery: go for it; there is still time to get it into... *checks*... C++23
10:01 < glozow> gracias jnewbery
10:01 < pg156> Thanks jnewbery!
10:01 < larryruane> thank you John, and everyone!! super helpful and fun!
10:01 < jnewbery> Thanks everyone. hope you all had fun!
10:01 < lsilva_> Thanks jnewbery
10:01 < michaelfolkson> Thanks!
10:01 < maxe> thank you jnewbery
10:01 < svav> Thanks jnewbery and all!
10:01 < effexzi> Thanks jnewbery 
10:01 -!- Azorcode [~Azorcode@186-90-4-130.genericrev.cantv.net] has quit [Quit: Connection closed]
10:01 < stickies-v> thanks everyone, great meeting again!
10:01 < jnewbery> I'm travelling next week, so I'm not able to host. I'm looking for a volunteer!
10:02 < gene> thanks jnewberry et al
10:02 < jnewbery> please message me if you'd like to host. If I can't find someone, we'll have to cancel next week's
10:02 < michaelfolkson> Hunger Games, Squid Game style fight to be host next week? :)
10:03 < larryruane> sipa: "... C++23" -- there must be some time-dilation going on, time is moving faster outside of C++ than inside!
10:03 < neha> thanks!
10:03 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has quit [Quit: Connection closed]
10:03 -!- maxe [~maxe@host-2-103-190-113.as13285.net] has quit [Quit: Connection closed]
10:03 < sipa> larryruane: since C++11 new language definition have been coming out every 3 years
10:04 -!- ragu [~ragu@pool-74-108-108-236.nycmny.fios.verizon.net] has quit [Client Quit]
10:06 < larryruane> sipa: +1 so they're basically on schedule, I wasn't sure about that
10:06 < gene> jnewberry: https://github.com/ignatz/pythonic
10:07 < larryruane> gene: wow that looks very cool
10:08 < gene> :) interesting that enumerated iterators weren't included in C++20
10:09 < gene> guess there was too much other good stuff
10:11 -!- Guest61 [~Guest61@124.123.169.87] has quit [Ping timeout: 256 seconds]
10:24 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews
10:37 -!- opcode_ [~opcode_@188.26.57.104] has joined #bitcoin-core-pr-reviews
10:38 < opcode_> Got late. Is there a public log archive?
10:47 < stickies-v> opcode_ the IRC logs always get added to the meeting notes afterwards, but I think it's a manual process so not sure what the time delay is. Keep an eye out on https://bitcoincore.reviews/23319 (e.g. previous meeting https://bitcoincore.reviews/23280 has the logs at the bottom)
10:49 < stickies-v> opcode_ I've quickly copied this meetings discussion on pastebin for you: https://pastebin.com/mQk7G9GK
10:50 < MarcoFalke> jnewbery: I can host next week, unless you found someone
10:55 < opcode_> stickies-v: appreciate it thanks
11:06 -!- ccdle12 [~ccdle12@243.222.90.149.rev.vodafone.pt] has quit [Quit: Client closed]
11:08 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has quit [Quit: https://quassel-irc.org - Chat comfortably. Anywhere.]
11:09 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews
11:10 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has quit [Client Quit]
11:11 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews
11:15 -!- opcode_ [~opcode_@188.26.57.104] has quit [Quit: Connection closed]
12:06 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
12:13 < jnewbery> MarcoFalke: that'd be great. Thank you!
12:14 < jnewbery> stickies-v opcode_: log is posted at https://bitcoincore.reviews/23319. I'll try to be more prompt in posting logs going forward
12:18 -!- effexzi [uid474242@ilkley.irccloud.com] has quit [Quit: Connection closed for inactivity]
12:44 -!- Common [~Common@096-033-221-075.res.spectrum.com] has joined #bitcoin-core-pr-reviews
12:44 -!- Common_ [~Common@096-033-221-075.res.spectrum.com] has joined #bitcoin-core-pr-reviews
12:45 -!- Common [~Common@096-033-221-075.res.spectrum.com] has quit [Changing host]
12:45 -!- Common [~Common@user/common] has joined #bitcoin-core-pr-reviews
12:46 -!- Common_ [~Common@096-033-221-075.res.spectrum.com] has quit [Client Quit]
12:55 -!- Common_ [~Common@096-033-221-075.res.spectrum.com] has joined #bitcoin-core-pr-reviews
12:56 -!- Common_ [~Common@096-033-221-075.res.spectrum.com] has quit [Client Quit]
13:00 -!- Common_ [~Common@096-033-221-075.res.spectrum.com] has joined #bitcoin-core-pr-reviews
13:00 -!- Common__ [~Common@096-033-221-075.res.spectrum.com] has joined #bitcoin-core-pr-reviews
13:00 -!- Common__ [~Common@096-033-221-075.res.spectrum.com] has quit [Remote host closed the connection]
13:04 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has quit [Ping timeout: 265 seconds]
13:36 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
13:40 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Ping timeout: 250 seconds]
14:09 -!- Common [~Common@user/common] has quit [Read error: Connection reset by peer]
14:09 -!- Common_ [~Common@096-033-221-075.res.spectrum.com] has quit [Read error: Connection reset by peer]
14:13 -!- Common [~Common@096-033-221-075.res.spectrum.com] has joined #bitcoin-core-pr-reviews
14:13 -!- Common [~Common@096-033-221-075.res.spectrum.com] has quit [Changing host]
14:13 -!- Common [~Common@user/common] has joined #bitcoin-core-pr-reviews
14:26 -!- Common [~Common@user/common] has quit [Read error: Connection reset by peer]
14:34 -!- Common [~Common@096-033-221-075.res.spectrum.com] has joined #bitcoin-core-pr-reviews
14:34 -!- Common [~Common@096-033-221-075.res.spectrum.com] has quit [Changing host]
14:34 -!- Common [~Common@user/common] has joined #bitcoin-core-pr-reviews
15:51 -!- grettke [~grettke@cpe-65-29-228-30.wi.res.rr.com] has quit [Quit: Textual IRC Client: www.textualapp.com]
16:08 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
16:13 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Ping timeout: 265 seconds]
16:20 -!- gene [~gene@gateway/tor-sasl/gene] has quit [Quit: gene]
16:51 -!- Common [~Common@user/common] has quit [Read error: Connection reset by peer]
16:52 -!- Common [~Common@096-033-221-075.res.spectrum.com] has joined #bitcoin-core-pr-reviews
16:54 -!- Common [~Common@096-033-221-075.res.spectrum.com] has quit [Read error: Connection reset by peer]
18:00 -!- b10c [uid500648@ilkley.irccloud.com] has quit [Quit: Connection closed for inactivity]
18:51 -!- virtu [~virtu@p54b61fd6.dip0.t-ipconnect.de] has quit [Ping timeout: 250 seconds]
18:53 -!- virtu [~virtu@p54b61958.dip0.t-ipconnect.de] has joined #bitcoin-core-pr-reviews
20:47 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
20:52 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Ping timeout: 265 seconds]
22:58 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has joined #bitcoin-core-pr-reviews
23:03 -!- Kaizen_Kintsugi_ [Kaizen_Kin@gateway/vpn/protonvpn/kaizenkintsugi/x-74018745] has quit [Ping timeout: 250 seconds]
23:22 -!- sekulicd [~sekulicd@79.140.150.38] has joined #bitcoin-core-pr-reviews
23:23 -!- sekulicd [~sekulicd@79.140.150.38] has quit [Client Quit]
--- Log closed Thu Nov 18 00:00:39 2021