--- Log opened Wed Sep 14 00:00:19 2022
01:22 -!- __gotcha1 [~Thunderbi@94.105.116.67.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
01:25 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Ping timeout: 264 seconds]
01:25 -!- __gotcha1 is now known as __gotcha
01:43 -!- af_mencken [~afmencken@69.4.234.83] has joined #bitcoin-core-pr-reviews
01:46 -!- __afmencken [~afmencken@69.4.234.76] has quit [Ping timeout: 265 seconds]
03:12 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Ping timeout: 252 seconds]
03:16 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
03:24 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Read error: Connection reset by peer]
03:25 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
04:39 -!- __gotcha1 [~Thunderbi@94.105.116.67.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
04:39 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Read error: Connection reset by peer]
04:39 -!- __gotcha1 is now known as __gotcha
04:47 -!- __gotcha1 [~Thunderbi@94.105.116.67.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
04:50 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Ping timeout: 265 seconds]
04:50 -!- __gotcha1 is now known as __gotcha
05:35 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:6dd4:ac97:5186:84c6] has joined #bitcoin-core-pr-reviews
06:28 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Remote host closed the connection]
06:29 -!- __gotcha1 [~Thunderbi@94.105.116.67.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
06:31 -!- __gotcha1 is now known as __gotcha
06:36 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Remote host closed the connection]
06:37 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
07:03 -!- __gotcha1 [~Thunderbi@94.105.116.67.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
07:04 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Ping timeout: 252 seconds]
07:04 -!- __gotcha1 is now known as __gotcha
07:09 -!- kouloumos [uid539228@id-539228.tinside.irccloud.com] has joined #bitcoin-core-pr-reviews
07:16 -!- __gotcha1 [~Thunderbi@94.105.116.67.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
07:16 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Read error: Connection reset by peer]
07:16 -!- __gotcha1 is now known as __gotcha
07:17 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Client Quit]
07:17 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
07:44 -!- tomi [~tomi@85.94.69.254] has joined #bitcoin-core-pr-reviews
07:44 -!- tomi [~tomi@85.94.69.254] has quit [Client Quit]
09:00 -!- mixoflatsixo[m] [~mixoflats@2001:470:69fc:105::1:2b63] has quit [Quit: You have been kicked for being idle]
09:14 < luke-jr> will I disrupt anything if I fix the bug? cc stickies-v
09:14 < instagibbs> raj-, -debug=1 gives you a nice firehose of everything too
09:19 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Ping timeout: 268 seconds]
09:21 < stickies-v> luke-jr: wouldn't want to hold you back! if the fix is minor it's definitely not a problem to fix already, if it's a significant rewrite perhaps would be nice to do it after the review club though?
09:26 < luke-jr> it's minor
09:26 < luke-jr> maybe I'll just do a fixup commit
09:40 -!- dulcedu [~dulcedu@172.92.166.62] has joined #bitcoin-core-pr-reviews
09:47 -!- TheSchnaz [~TheSchnaz@c-98-218-146-119.hsd1.va.comcast.net] has joined #bitcoin-core-pr-reviews
09:49 -!- Amirreza [~Amirreza7@2.177.104.105] has joined #bitcoin-core-pr-reviews
09:54 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
09:55 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has joined #bitcoin-core-pr-reviews
09:57 -!- ccdle12 [~ccdle12@243.222.90.149.rev.vodafone.pt] has joined #bitcoin-core-pr-reviews
10:00 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has joined #bitcoin-core-pr-reviews
10:00 -!- alecc [~alecc@s-25-172.flex.volo.net] has joined #bitcoin-core-pr-reviews
10:00 < stickies-v> #startmeeting
10:00 < larryruane_> hi!
10:00 < glozow> hi
10:01 < alecc> hi
10:01 -!- asi0 [~asi0@liphy-dyfcom-wlubuntu.u-ga.fr] has joined #bitcoin-core-pr-reviews
10:01 < stickies-v> welcome everyone! Today we're looking at #25991, authored by luke-jr. The notes and questions are available on https://bitcoincore.reviews/25991
10:01 < Amirreza> Hi
10:02 < stickies-v> anyone joining us for the first time today? even if you're just lurking, feel free to say hi! it's nice to see who's following
10:02 < alecc> this is my first time!
10:02 < luke-jr> me too! <.<
10:02 < luke-jr> maybe
10:02 < brunoerg> hi
10:02 < asi0> hi! Second time here but still lurking ^^'
10:03 < stickies-v> welcome alecc, glad that you found your way here! don't hold back to raise any questions you have, it's a very welcoming environment here
10:03 < luke-jr> btw, in the process of fixing the minor bug, I found another less trivial-to-fix one ;)
10:03 < stickies-v> haha and welcome to our PR author and (maybe) first-time attendee luke-jr, thank you for joining us!
10:04 -!- richardforthrast [~richardfo@c-24-9-105-48.hsd1.co.comcast.net] has joined #bitcoin-core-pr-reviews
10:04 < stickies-v> who got the chance to review the PR or read the notes? (y/n)
10:04 < alecc> y
10:04 < brunoerg> I just read the notes
10:04 -!- gibbons [~gibbons@37.19.221.174] has joined #bitcoin-core-pr-reviews
10:05 < stickies-v> luke-jr: well definitely do feel free to bring it up in this session too if it's interesting to discuss!
10:05 < Amirreza> y
10:05 < asi0> same as brunoerg
10:06 < alecc> i didn't review on github but went through the notes/questions
10:06 < luke-jr> stickies-v: should we wait to see if someone else notices it? :P
10:07 < stickies-v> haha we can keep it as a bonus until the end to keep people hooked
10:07 < stickies-v> what are everyone's general thoughts on the PR? and if you have been able to review in more detail, would you give it a Concept ACK, Approach ACK, Tested ACK, or NACK?
10:07 < glozow> luke-jr: records say you have attended before :P welcome back! https://bitcoincore.reviews/17428#l-16
10:08 -!- chipxxx [~chip@pop.92-184-123-169.mobile.abo.orange.fr] has joined #bitcoin-core-pr-reviews
10:08 < luke-jr> :o
10:09 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has quit [Quit: Connection closed]
10:09 < alecc> stickies-v: concept ACK - fixing fee calculation makes sense, i'm not super certain on implementation mostly because i'm new to the codebase
10:10 < Amirreza> Can someone explain to me what does `Cache` means in the context of wallet code? I don't understand for example `GetCachableAmount` or `CachedTxGetDebit`?
10:11 < larryruane_> concept ACK, definitely would be good to fix, still studying the approach and the code
10:11 -!- helloworld [~helloworl@190.62.9.151] has joined #bitcoin-core-pr-reviews
10:11 < stickies-v> alecc: just to avoid confusion, the issue fixed here is not about wrong fee calculation when creating a transaction (which would cost the user money), but rather about displaying/calculating fees for existing transactions in a users wallet
10:11 -!- helloworld [~helloworl@190.62.9.151] has quit [Client Quit]
10:12 < luke-jr> arguably the current logic is even correct - it just can't handle the data it doesn't know
10:12 < stickies-v> yes, sorry - that's a good nuance, thanks
10:12 < alecc> stickies-v: makes sense, thanks for clarifying
10:13 < stickies-v> Amirreza: I'm actually not sure why all these functions contain Cache, hopefully someone more familiar with the wallet can clarify?
10:13 < stickies-v> awaiting that, let's dive into the questions
10:13 -!- gibbons [~gibbons@37.19.221.174] has quit [Quit: Connection closed]
10:13 < stickies-v> quick reminder - i'll be iterating over the questions sequentally, but the discussion can go async, so feel free to refer to previous questions or any other relevant topics you want to discuss
10:13 < michaelfolkson> hi
10:14 < stickies-v> We already have `CWallet::IsMine()` - why do we need the new function `CWalletTx::IsForeignOutput()`? What is the difference between these functions? What is a foreign output?
10:14 < stickies-v> maybe let's focus on the last bit first. conceptually, what is a foreign output?
10:15 < luke-jr> right now, the code assumes that if any of the inputs came from this wallet, all outputs to the transaction are sends from this wallet
10:15 < luke-jr> in the case of a CoinJoin, that assumption is incorrect
10:15 < luke-jr> but the wallet doesn't know which outputs it DID send, or didn't
10:15 < alecc> the PR notes specifically talk about CoinJoin - I was thinking a foreign output was an output that is 1. not being sent to a pubkey owned by our wallet and 2. not created by our wallet?
10:15 < luke-jr> "foreign output" is the term I'm using for outputs that contradict the assumption - that is, they AREN'T sent by this wallet
10:16 < luke-jr> it _could_ also still be _to_ this wallet, and if so should be treated as a receive
10:17 < alecc> when we say "sent by this wallet", that's like if we were participating in a coin join but didn't contribute this output? i imagine it's more general than that maybe
10:17 < luke-jr> right, that's the general example
10:17 < stickies-v> alecc: the second part of your statement is particularly relevant
10:17 < larryruane_> so a single transaction could have a mixture of foreign and non-foreign outputs?
10:17 < luke-jr> though "contribute" can be confusing
10:17 < luke-jr> larryruane_: yes, that's the expectation
10:18 < luke-jr> in fact, it could break things in other ways, if all the outputs get marked as foreign
10:18 < luke-jr> but it's not really clear how that _should_ work, so I'd treat that scenario as UB
10:18 < stickies-v> (UB is Undefined Behaviour, for anyone unfamiliar with the term)
10:19 < alecc> thanks, what just about to ask
10:19 < stickies-v> thanks for that context, luke-jr 
10:19 < luke-jr> yeah, sorry
10:20 < larryruane_> oh I thought UB had to do only with the details of our C++ code, but the term also applies to how overall bitcoin works?
10:20 -!- Amirreza [~Amirreza7@2.177.104.105] has quit [Quit: Leaving]
10:20 < larryruane_> (if so that's interesting!)
10:21 < stickies-v> just to rephrase it a bit, IsMine() tells us if a tx/script/output etc is recognized (e.g. it's watching it, or it can spend it) by our wallet
10:21 < luke-jr> larryruane_: UB is generic; you can use it to refer to things in the real world too :P
10:21 < stickies-v> however, we need to keep in mind that in a bitcoin transaction, an output doesn't specify which input(s) it spends. conceptually, it's like all the inputs are pooled together and then the output(s) just spend from that pool
10:21 < luke-jr> the actions of a toddler are often UB
10:22 < larryruane_> luke-jr: +1 TIL thanks
10:22 < larryruane_> haha!
10:22 < sipa> eh, UB is a specific term in the C/C++ standards
10:22 -!- richardforthrast [~richardfo@c-24-9-105-48.hsd1.co.comcast.net] has quit [Quit: Connection closed]
10:22 < sipa> sure, you can use it in the real world too, like many terms of art
10:22 < sipa> but it has a well-defined (ha!) meaning within the context of the C and C++ languages
10:23 < alecc> stickies-v: given that, i'm a little confused on what actually qualifies an output to be foreign? from the code i didn't see anything "detecting" an output to be foreign/don't know how it would be i guess
10:23 < luke-jr> alecc: it's not detectable; the user must provide the information
10:23 < stickies-v> so when looking at a transaction's outputs as an external observer, we can't really say "who" funds each output. we need context to be able to do that
10:24 < alecc> luke-jr: ah i was thinking that but wasn't sure
10:24 < luke-jr> this PR just makes it possible for the user to do so, saves it, and interprets using it
10:24 < stickies-v> alecc: foreign means, in simple terms, that you have "nothing to do" with that output. someone else is funding it, you just all happen to be in the same PR
10:24 < stickies-v> *same transaction, not PR
10:24 < sipa> luke-jr: Specifically, I'd say that foreign outputs thing matches more the "unspecified value" in C/C++ (which means the implementation could have many valid behaviors, which aren't well defined, but it can't do completely unrelated things like wipe your wallet.dat file)
10:24 < alecc> stickies-v: gotcha
10:25 < luke-jr> sipa: ok
10:25 < stickies-v> okay hopefully that clears up the context/purpose of the PR for everyone, I'll move on to the next question
10:25 < stickies-v> Does commit "Wallet: Refactor CachedTxGetAmounts fee calculation to inline value_out" introduce any behaviour change, or is it just refactoring?
10:25 < stickies-v> (link: https://github.com/bitcoin/bitcoin/pull/25991/commits/efa22dd36f1399c49c5a149ace5232b700c7b049)
10:26 -!- BlueMoon [~BlueMoon@dgb3.dgbiblio.unam.mx] has joined #bitcoin-core-pr-reviews
10:26 -!- OxBEEFCAF3 [~OxBEEFCAF@c-73-149-241-246.hsd1.ma.comcast.net] has joined #bitcoin-core-pr-reviews
10:26 < BlueMoon> Hello, sorry, I got in late, I was busy.
10:27 < alecc> stickies-v: from what i could tell this commit alone is just a refactor - changes fee calculation from using `tx-GetValueOut` to manually adding them to a total when iterating through outputs
10:27 -!- richardforthrast [~richardfo@c-24-9-105-48.hsd1.co.comcast.net] has joined #bitcoin-core-pr-reviews
10:28 < stickies-v> alecc: can you see any difference in the implementation of `GetValueOut()` and the 'inlined' version from this commit, though?
10:28 < alecc> oh yea `GetValueOut` has some constraints on the value i think
10:29 < alecc> that's actually probably pretty important isn't it?
10:29 < stickies-v> yup exactly, it checks that the output amount is <21m btc by using the `MoneyRange()` function
10:29 < alecc> `MoneyRange`
10:29 < alecc> oh dang
10:30 < stickies-v> so, is this behaviour change? and/or is it safe to do?
10:30 < alecc> yes behaviour change, not safe
10:32 < luke-jr> seeing as this is only called on a wallet tx, I would argue it's safe
10:33 < luke-jr> also note being outside the range throws an exception (or asserts), which would break things like listtransactions entirely
10:33 < stickies-v> luckily, it's both safe and no behaviour change. earlier in the function, when we calculate `nDebit`, `CachedTxGetDebit` (well, the functions it calls) has already checked the output values of the tx through `MoneyRange`
10:33 < luke-jr> but perhaps a reviewer ought to check call sites for the function
10:34 < luke-jr> stickies-v: ah, I didn't notice that ☺
10:34 < stickies-v> so effectively this inlined version just removed the duplicated (and quite possibly the ranges have been checked in other places already as well) check. all is well, another day without inflation loopholes!
10:35 < alecc> mm makes sense, i jumped to the conclusion too quickly
10:36 < alecc> now that I'm actually looking at how `MoneyRange` is used in `GetValueOut` it looks like it's checking just that the txout total value is within 0-21mil which seems kinda redundant? like it's checking that the tx isn't literally spending all possible btc? maybe a question for another time
10:38 < stickies-v> I'm not 100% all the places where MoneyRange is used or its exact purpose. Could be used as protection against inflation, but also against e.g. buffer overflow. I believe that's a bug we had quite a few years ago.
10:39 < stickies-v> Alright, next question (changing the order a bit)
10:39 < stickies-v> `CWalletTx::m_foreign_outputs` is implemented as a `std::vector<bool>`. Is there anything peculiar about `std::vector<bool>`? Which other data structure(s) would you consider using instead, if any - and why?
10:39 < stickies-v> (link: https://github.com/bitcoin-core-review-club/bitcoin/blob/10bbb0a5252470c5afe17c38326476be4a523613/src/wallet/transaction.h#L193)
10:41 < alecc> this one i wasn't too sure on - i'd have to look back in the code to see the context more, but if you know how many outputs you have before you create the `m_foreign_ouputs` vector, then you could maybe make it an array/don't need the resizeability
10:42 < michaelfolkson> The bool only uses a bit of space rather than a byte https://stackoverflow.com/questions/17794569/why-isnt-vectorbool-a-stl-container
10:43 < michaelfolkson> "doesn't offer all the capabilities and interface of a normal standard container"
10:44 < alecc> oh woah
10:44 < luke-jr> michaelfolkson: not guaranteed to, iirc, but hopefully
10:44 < stickies-v> michaelfolkson: yeah exactly, even though the spec doesn't guarantee std::vector<bool> to be more space efficient, most (all?) implementations do, but at the cost of a different interface and some unexpected behaviour, e.g. you can't use pointers to the elements of a bool vector
10:45 < stickies-v> in many cases it's not a problem, just something to be aware of and consider, which is why I included it in the questions here
10:45 < luke-jr> so it's actually more efficient than putting a bool on CTxOut (which would kinda be a layering issue)
10:45 < willcl_ark> seems like boost.container has a regular bool vec
10:46 < stickies-v> willcl_ark: but at the same time we're also trying to remove boost dependencies, unfortunately :-D
10:46 < luke-jr> willcl_ark: we prefer STL over boost
10:46 < michaelfolkson> luke-jr: Not guaranteed to only use a bit of space, you mean?
10:46 < luke-jr> michaelfolkson: right
10:46 < willcl_ark> sure and I agree! Was just looking for alternatives as per the q :P
10:47 < stickies-v> from what I've read, most common alternatives are std::vector<char> or std::bitset - but I'm not sure these would be preferable in this situation?
10:47 < stickies-v> willcl_ark: yes sorry, thank you for the input!
10:48 < willcl_ark> deque? IMO it's fine to use the weird bool vec here though 
10:48 < larryruane_> wow TIL ... I'd think it would have been better to have std::vector<bool> just be (basically) an array of bytes, and have a specific bitmap class... rather than having that exception
10:48 -!- richardforthrast [~richardfo@c-24-9-105-48.hsd1.co.comcast.net] has quit [Quit: Connection closed]
10:49 < larryruane_> I wonder, if you wanted the each-bool-is-a-byte type of container, if you could declare a `std::vector<uint8_t>` or something, and just always store 0 or 1 in its elements
10:49 < stickies-v> larryruane_: when perusing stackoverflow, you'll find many people share your sentiment. hard to phase out once it's in the standard library, though
10:50 < stickies-v> larryruane_:  yeah exactly that's a valid alternative
10:51 < stickies-v> Next up: what is the purpose of the for-loop in `CWalletTx::Serialize()`? For i=3, what is the value of `1 << (i % 8)` - and what does it mean? 
10:51 < stickies-v> (Hint: what does the `[]` operator on a `std::string` return, and what is the size of that return value?)
10:51 < stickies-v> (link: https://github.com/bitcoin-core-review-club/bitcoin/blob/10bbb0a5252470c5afe17c38326476be4a523613/src/wallet/transaction.h#L246-L248)
10:52 < alecc> it looks like it packs the values into a sequence of bits - iterates over each char/byte in the string 8 times for each bit
10:53 < alecc> for i = 3 `1 << (i % 8)` = 0001000 = 8
10:54 < stickies-v> alecc:  yeah you got it!
10:54 < stickies-v> so, if we're iterating over the same char multiple times... aren't we just overwriting it the whole time?
10:55 < larryruane_> why `m_foreign_outputs.at(i)` instead of `m_foreign_outputs[i]` -- in this situation would those be the same?
10:56 < stickies-v> larryruane_: that's one of the review comments I've got lined up hah. since we've checked bounds already, I think we should just use `[]` which is slightly faster
10:56 < alecc> stickies-v: we're bitwise or-ing the byte with a number that only has as most 1 nonzero bit, so it's only flipping that individual bit each iteration
10:56 < luke-jr> stickies-v: is it?
10:57 < stickies-v> alecc: yeah!
10:57 < stickies-v> luke-jr: is it faster? I've not benchmarked, but... that's what I read? which would make sense since we're not redoing the bounds checking first?
10:58 < luke-jr> stickies-v: it might be. other C++ stuff (maybe just maps?) generates "create a new item" and such for operator[], in which case .at() is cheaper for read access
10:59 < larryruane_> "aren't we just overwriting it" -- I think so, an alternative might have been to write to a stack variable (in practice probably a register) instead of an element of the string `s` and only store it to `s` after the loop.. but I don't know if it's worth it
11:00 < luke-jr> stickies-v: looks like you're right
11:00 < stickies-v> larryruane_: sorry yeah you're right, we're overwriting the byte but keeping the individual bits which is what I meant, but you're definitely right with that nuance (and potential performance implications it may have, although probably not that important)
11:01 < stickies-v> luke-jr: okay thanks I didn't know that, will have to read up on it a bit!
11:01 < stickies-v> alright folks looks like we're at time, that's all for today - thank you very much for attending and participating!
11:01 < stickies-v> #endmeeting
11:01 < glozow> thank you stickies-v!
11:01 < alecc> thanks everyone! thanks stickies-v!
11:01 < stickies-v> and special thank you to luke-jr for authoring the PR and sharing his wisdom here with us today, much appreciated
11:02 < larryruane_> Just to check something, if the constructor `std::string s(m_foreign_outputs.size(), 0);`, if the 0 arg wasn't given, then the values wouldn't be guaranteed to be zero? 
11:02 -!- alecc [~alecc@s-25-172.flex.volo.net] has quit [Quit: Connection closed]
11:02 < larryruane_> thanks @stickies-v and @luke-jr !
11:02 < luke-jr> btw, the bug is that the modified transaction was never being updated in the wallet file
11:03 < luke-jr> and yet another bug: reading it back out could make the m_foreign_outputs vector larger than the output count, which failed another assert
11:03 < luke-jr> pushed a bunch of fixups to the PR, will probably squash them in a bit, but I'll leave it for people to look over for now
11:04 < stickies-v> oh the last question actually covered disk persistence, shame we didn't get to it
11:04 < luke-jr> larryruane_: I think 0 is required there
11:04 < michaelfolkson> Thanks stickies-v luke-jr!
11:04 < luke-jr> larryruane_: https://cplusplus.com/reference/string/string/string/
11:05 < glozow> next week is testing v24 release candidates. Looks like kouloumos has already put up a testing guide (notes will be up later as well): https://gist.github.com/kouloumos/fc112640a533e522d435c0995dcaaaf4
11:05 -!- evanlinjin [~root@gateway/tor-sasl/evanlinjin] has quit [Ping timeout: 258 seconds]
11:07 < larryruane_> luke-jr: thanks, and @stickies-v on the pre- and post-increment, it seems that preincrement is faster, which is maybe the guidelines suggest using it, compare these: https://godbolt.org/z/zsG19s78G https://godbolt.org/z/f7Y6Ezb8n although just the instruction count probably isn't a reliable way to conclude anything about performance
11:07 < larryruane_> glozow: thanks, this should be a good one!
11:08 < luke-jr> larryruane_: this one depends on the postincrement behaviour, though marco seems annoyed by it
11:09 < luke-jr> or postdecrement, rather :P
11:09 < larryruane_> luke-jr: I was going to ask about his comment ... Is that warning he mentioned something that will keep popping up as long as the code is as you wrote it? Like in CI for example? If so, would that be a good reason to "fix" it?
11:10 < luke-jr> disable the warning, I guess
11:10 < luke-jr> not sure if there's a good way to "fix" it
11:10 < luke-jr> I suppose just checking i there, and decrementing in the loop code
11:10 < stickies-v> larryruane_: I believe besides performance considerations (I believe pre is more efficient - or never less efficient - because post requires to create a copy or pointer?), pre is also recommended because the behaviour of post is a bit less straightforward/surprising (which is relative to the reviewer, of course)
11:11 -!- ccdle12 [~ccdle12@243.222.90.149.rev.vodafone.pt] has quit [Quit: Connection closed]
11:11 < stickies-v> luke-jr: wouldn't `for (auto i = tx->vout.size(); i>=0; --i) {` be an alternative?
11:11 < luke-jr> stickies-v: that is an infinite loop
11:11 < luke-jr> unsigned types are always >=0
11:12 < stickies-v> oh yes of course, woops
11:14 < larryruane_> another problem with that is the first iteration of the loop has i out of bounds (one too high)
11:15 < larryruane_> you'd want to start at size() - 1 (but still has the problem Luke mentioned)
11:16 < luke-jr> for (auto i = tx->vout.size(); i>=1; ) { --i;
11:16 < luke-jr> maybe
11:16 -!- asi0 [~asi0@liphy-dyfcom-wlubuntu.u-ga.fr] has quit [Quit: Connection closed]
11:16 < stickies-v> or cast i to int?
11:16 < larryruane_> I guess what you could do is `for (auto i = tx->vout.size(); i; --i) {` but then always use `i-1` in the body of the loop
11:16 -!- evanlinjin [~root@gateway/tor-sasl/evanlinjin] has joined #bitcoin-core-pr-reviews
11:17 < larryruane_> (or have another variable that you just initialize to `i-1` just inside the loop body, and use that, never `i`
11:17 < larryruane_> actually I think that may be better, what do you think, @luke-jr ? (I could leave as a review comment)
11:18 < luke-jr> those both feel uglier than i>=1 { --i IMO
11:18 < luke-jr> kindof a shame, since the current one is "perfect" for the purpose
11:20 < larryruane_> yeah it is okay as it is.. maybe even `for (auto j = 0; j < tx->vout.size(); ++j) { i = tx->vout.size() - j - 1; ... (use i) ...` nice to have a cononical loop construct but kind of ugly too
11:22 < larryruane_> (look inefficient but I'm sure `size()` is inlined and would be cached in a register)
11:27 < luke-jr> it doesn't have to be in any particular order
11:27 < luke-jr> but I'm not sure size can be cached?
11:30 -!- __afmencken [~afmencken@69.4.234.100] has joined #bitcoin-core-pr-reviews
11:32 -!- af_mencken [~afmencken@69.4.234.83] has quit [Ping timeout: 252 seconds]
11:34 -!- BlueMoon [~BlueMoon@dgb3.dgbiblio.unam.mx] has quit [Quit: Connection closed]
11:52 -!- OxBEEFCAF3 [~OxBEEFCAF@c-73-149-241-246.hsd1.ma.comcast.net] has quit [Ping timeout: 260 seconds]
12:00 -!- richardforthrast [~richardfo@c-24-9-105-48.hsd1.co.comcast.net] has joined #bitcoin-core-pr-reviews
12:00 -!- OxBEEFCAF3 [~OxBEEFCAF@c-73-149-241-246.hsd1.ma.comcast.net] has joined #bitcoin-core-pr-reviews
12:05 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
12:08 -!- steve785 [~steve785@pool-96-255-32-133.washdc.fios.verizon.net] has joined #bitcoin-core-pr-reviews
12:09 -!- steve785 [~steve785@pool-96-255-32-133.washdc.fios.verizon.net] has quit [Client Quit]
12:11 -!- steve785 [~steve785@pool-96-255-32-133.washdc.fios.verizon.net] has joined #bitcoin-core-pr-reviews
12:12 -!- steve785 [~steve785@pool-96-255-32-133.washdc.fios.verizon.net] has quit [Client Quit]
12:24 -!- TheSchnaz [~TheSchnaz@c-98-218-146-119.hsd1.va.comcast.net] has quit [Quit: Connection closed]
12:27 -!- richardforthrast [~richardfo@c-24-9-105-48.hsd1.co.comcast.net] has quit [Quit: Connection closed]
12:31 -!- OxBEEFCAF3 [~OxBEEFCAF@c-73-149-241-246.hsd1.ma.comcast.net] has quit [Quit: Connection closed]
12:55 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has quit [Quit: Connection closed for inactivity]
13:02 -!- chipxxx [~chip@pop.92-184-123-169.mobile.abo.orange.fr] has quit [Read error: Connection reset by peer]
13:25 -!- jesseposner [~jesse@user/jesseposner] has quit [Quit: Textual IRC Client: www.textualapp.com]
13:29 -!- jesseposner [~jesse@user/jesseposner] has joined #bitcoin-core-pr-reviews
13:29 -!- jesseposner [~jesse@user/jesseposner] has quit [Client Quit]
13:31 -!- af_mencken [~afmencken@69.4.234.69] has joined #bitcoin-core-pr-reviews
13:34 -!- __afmencken [~afmencken@69.4.234.100] has quit [Ping timeout: 260 seconds]
13:35 -!- __afmencken [~afmencken@69.4.234.76] has joined #bitcoin-core-pr-reviews
13:38 -!- af_mencken [~afmencken@69.4.234.69] has quit [Ping timeout: 265 seconds]
13:38 -!- afmencken [~afmencken@69.4.234.100] has joined #bitcoin-core-pr-reviews
13:40 -!- __afmencken [~afmencken@69.4.234.76] has quit [Ping timeout: 265 seconds]
13:58 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
14:07 -!- jesseposner [~jesse@user/jesseposner] has joined #bitcoin-core-pr-reviews
14:48 -!- kouloumos [uid539228@id-539228.tinside.irccloud.com] has quit [Quit: Connection closed for inactivity]
14:57 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Ping timeout: 265 seconds]
15:12 -!- Zenton [~user@user/zenton] has quit [Read error: Connection reset by peer]
15:14 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:6dd4:ac97:5186:84c6] has quit []
15:25 -!- dulcedu [~dulcedu@172.92.166.62] has quit [Quit: Connection closed]
16:31 -!- S3RK_ [~S3RK@user/s3rk] has quit [Ping timeout: 255 seconds]
16:34 -!- S3RK [~S3RK@user/s3rk] has joined #bitcoin-core-pr-reviews
17:25 -!- drnet [~drnet@77.119.216.147.wireless.dyn.drei.com] has joined #bitcoin-core-pr-reviews
17:49 -!- drnet [~drnet@77.119.216.147.wireless.dyn.drei.com] has quit [Ping timeout: 264 seconds]
18:11 -!- drnet [~drnet@77.119.216.147.wireless.dyn.drei.com] has joined #bitcoin-core-pr-reviews
18:21 -!- drnet [~drnet@77.119.216.147.wireless.dyn.drei.com] has quit [Read error: Connection reset by peer]
18:21 -!- drnet [~drnet@77.119.216.147.wireless.dyn.drei.com] has joined #bitcoin-core-pr-reviews
18:36 -!- drnet [~drnet@77.119.216.147.wireless.dyn.drei.com] has quit [Ping timeout: 244 seconds]
19:35 -!- hashfunc [~user@2601:5c0:c280:7090:83:ebf7:f91d:450a] has joined #bitcoin-core-pr-reviews
20:18 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews
23:33 -!- drnet [~drnet@185.89.163.206] has joined #bitcoin-core-pr-reviews
23:35 -!- drnet [~drnet@185.89.163.206] has quit [Client Quit]
23:43 -!- phogster [~roger@47.144.174.140] has joined #bitcoin-core-pr-reviews
23:44 -!- phogster [~roger@47.144.174.140] has quit [Quit: Leaving]
--- Log closed Thu Sep 15 00:00:20 2022