< bitcoin-git> [bitcoin] zachwylde00 opened pull request #18237: Create circle.ci.yml (master...master) https://github.com/bitcoin/bitcoin/pull/18237
< bitcoin-git> [bitcoin] fanquake closed pull request #18237: Create circle.ci.yml (master...master) https://github.com/bitcoin/bitcoin/pull/18237
< kallewoof> So, bitcoin core right now seems to lean towards constant func/method parameters to pointers in the form "type* const" (55 instances of "CWallet* const pwallet") over "const type*" (15 instances of "const CWallet*"), but from what I understand, "type* const" means the pointer itself is const (i.e. "pwallet = blarf" is illegal) but the pointed to instance is not const (i.e. pwallet->MutableThing() is legal). I tested this
< kallewoof> by adding a call to pwallet->handleNotifications(), a non-const marked function, and it compiles w/o issue.
< kallewoof> Switching the parameter to "const CWallet* pwallet" and recompiling gave an error, so yeah, those type* const declarations are all pretty much pointless.
< sipa> kallewoof: that's correct
< sipa> i wouldn't say it's "leaning towards"... they're completely different things
< sipa> with different meanings
< sipa> if someone wrote on while meaning the other, that's a bug
< sipa> you can also have "const Type* const varname" to make both the variable amd the pointed-to object const
< kallewoof> I can't see why "retval method(CWallet* const pwallet, ...)" would ever mean "let's not change the pwallet pointer"
< sipa> i don't understand; that is what it means
< kallewoof> Yeah I mean, I can't see the reason why anyone would wanna do that
< kallewoof> The thing expires on return anyway. If you really want to a CWallet pointer to something else, just make a new one. That's better practice anyway.
< sipa> some people like making parameters const-valued if they know the function body won't change it
< sipa> in general
< sipa> i don't (i don't think it adds much), but it's not unreasonable to mark everything you know to be const as const
< kallewoof> Yeah, that's a fair point. I do think this is easy to mis(s/understand). I did, until I looked it up again for the Nth time.
< sipa> maybe it's worth trying to make the CWallet const as well in those 55 instances
< sipa> and see if it still compiles in some of the.
< kallewoof> I'd say that the majority of those 55 "CWallet* const" parameters were indeed meant to be "const CWallet*", but I could be mistaken.
< kallewoof> Yeah, I'll give it a shot
< sipa> if so, that would be strong indication that those const markers were added in error
< luke-jr> kallewoof: the reason is so you don't accidentally change the param
< luke-jr> I certainly mean *const when I put *const :p
< fanquake> kallewoof looks like similar comments have been made before: https://github.com/bitcoin/bitcoin/pull/8775#discussion_r103283605
< kallewoof> luke-jr: you mean, so you don't accidentally set e.g. pwallet to something else while inside the method? sounds like a rather obvious thing to spot, though, but safe > sorry. I think the drawback in this case is that people who add code will see a bunch of "CWallet* const" declarations and assume they mean "immutable CWallets" when they mean "immutable pointers to mutable CWallets".
< aj> better to use "Foo&" than "Foo* const" for a class member, isn't it?
< sipa> aj: ifyou k
< sipa> if you know it can't be nullptr
< aj> fair point
< sipa> also if you store references in a class and also accept a reference type in the constructor to build it with, it may not be clear to callers that the new object's lifetime is limited to that of the argument
< sipa> if you pass a pointer, it's generally more obvious (to me, at least) that you need to be careful
< bitcoin-git> [bitcoin] ajtowns opened pull request #18238: net_processing: Retry notfounds with more urgency (master...202002-bump-notfound) https://github.com/bitcoin/bitcoin/pull/18238
< bitcoin-git> [bitcoin] promag opened pull request #18239: gui: Refactor to drop client and wallet models setters (master...2020-03-drop-setmodel) https://github.com/bitcoin/bitcoin/pull/18239
< promag> wumpus: pushed #18239 to address your comment in #18064 - if you agree with the concept then #18064 can already be merged given the existing acks
< gribble> https://github.com/bitcoin/bitcoin/issues/18239 | wip: gui: Refactor to drop client and wallet models setters by promag · Pull Request #18239 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/18064 | gui: Drop WalletModel dependency to RecentRequestsTableModel by promag · Pull Request #18064 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/18064 | gui: Drop WalletModel dependency to RecentRequestsTableModel by promag · Pull Request #18064 · bitcoin/bitcoin · GitHub
< jonasschnelli> how do I get the fuzzer to compile on macOS?
< jonasschnelli> just dropping a --enable-fuzz leads to linker errors on my end
< fanquake> jonasschnelli I'd suggest using a brew installed llvm/clang 9 if you aren't already, rather than Apple Clang.
< fanquake> What errors are you seeing?
< jonasschnelli> fanquake: ld: entry point (_main) undefined. for architecture x86_64
< fanquake> jonasschnelli is this on latest master? I fixed some fuzzing issues related to main() and libfuzzer in #18008.
< gribble> https://github.com/bitcoin/bitcoin/issues/18008 | test: only declare a main() when fuzzing with AFL by fanquake · Pull Request #18008 · bitcoin/bitcoin · GitHub
< jonasschnelli> fanquake: Yes. Master. I never used the fuzzers... so could be dumb me
< jonasschnelli> I'll try now with brews clang
< fanquake> Ok. That should work well. Using a ./configure like
< fanquake> ./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined CC=/usr/local/opt/llvm/bin/clang CXX=/usr/local/opt/llvm/bin/clang++
< jonasschnelli> fanquake: with brews llvm packaged it worked. Thanks.
< fanquake> jonasschnelli: np
< bitcoin-git> [bitcoin] meshcollider pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/54a7ef612a3b...1f886243e464
< bitcoin-git> bitcoin/master 1ef28b4 Gregory Sanders: Make AnalyzePSBT next role calculation simple, correct
< bitcoin-git> bitcoin/master 1f88624 Samuel Dobson: Merge #18224: Make AnalyzePSBT next role calculation simple, correct
< bitcoin-git> [bitcoin] meshcollider merged pull request #18224: Make AnalyzePSBT next role calculation simple, correct (master...analyze_psbt_role_simple) https://github.com/bitcoin/bitcoin/pull/18224
< kallewoof> sipa, aj, luke-jr: for the record, changing to immutable-CWallet-pointers in places where it made sense (many places did non-const stuff) resulted in 68 changes; see https://github.com/bitcoin/bitcoin/compare/master...kallewoof:2002-const-fixes ... is this something people would concept ack on a PR?
< sipa> sounds good to me; this is the sort of change that as long as it compiles, the behavior shouldn't change
< sipa> though in general it may lead to introducing automatic copying of objects sometimes (e.g. trying to std::move a const object will work, but generally result in a copy rather than an efficient move)
< sipa> CWallet objects aren't copied or moved though
< kallewoof> sipa: ohh, good point.
< bitcoin-git> [bitcoin] kallewoof opened pull request #18241: wallet/refactor: refer to CWallet immutably when possible (master...2002-const-fixes) https://github.com/bitcoin/bitcoin/pull/18241
< jonasschnelli> why do I get a asan heap buffer overflow detection in ParseHex (vch.push_back(n);)?
< promag> jonasschnelli: not sure, proly due to `while (true)`?
< jonasschnelli> promag: I don't know. I though we use the same env (clang/asan) on travis
< jonasschnelli> *thought
< promag> but you get am error or warning?
< kallewoof> Tried -fsanitize=address but compiler errors on crypto/sha256_sse4.cpp:44, saying "expected relocatable expression". Should probably read up on how it works somewhere.
< fanquake> kallewoof: you might have to pass --disable-asm to configure
< fanquake> iirc there's a note about a similar issue in our docs
< bitcoin-git> [bitcoin] jonasschnelli opened pull request #18242: Add BIP324 encrypted p2p transport de-/serializer (only used in tests) (master...2020/03/net_v2) https://github.com/bitcoin/bitcoin/pull/18242
< kallewoof> fanquake: aha! thanks
< instagibbs> would people accept a "mockfeeestimation" RPC that would let the test-writer hardcode the rates? Looking at a reported issue and without this or something like it it makes writing tests difficult
< instagibbs> obviously only active on test-related networks, hidden blah blah
< instagibbs> issue is with respect to bumpfee*
< provoostenator> instagibbs: sounds good to me
< provoostenator> Makes sense for Signet too
< instagibbs> for now I hijacked feature_estimate_fee.py to write additional test cases :P
< instagibbs> opened an issue
< bitcoin-git> [bitcoin] laanwj pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/1f886243e464...ac5c5d0162a9
< bitcoin-git> bitcoin/master aff2748 Filip Gospodinov: httpserver: use own HTTP status codes
< bitcoin-git> bitcoin/master ac5c5d0 Wladimir J. van der Laan: Merge #18168: httpserver: use own HTTP status codes
< bitcoin-git> [bitcoin] laanwj merged pull request #18168: httpserver: use own HTTP status codes (master...500) https://github.com/bitcoin/bitcoin/pull/18168
< wumpus> instagibbs: if it helps increasing test coverage and it doesn't complicate the non-test code paths too much it sounds like a good idea
< bitcoin-git> [bitcoin] Sjors opened pull request #18244: rpc: have lockUnspents also lock manually selected coins (master...2020/03/rpc_coin_locks) https://github.com/bitcoin/bitcoin/pull/18244
< bitcoin-git> [bitcoin] instagibbs opened pull request #18245: Test some transaction creation with non-empty fee estimator (master...test_est_txn) https://github.com/bitcoin/bitcoin/pull/18245
< bitcoin-git> [bitcoin] promag opened pull request #18246: gui: Drop connectSlotsByName usage (master...2020-03-drop-connectslotsbyName) https://github.com/bitcoin/bitcoin/pull/18246
< bitcoin-git> [bitcoin] MarcoFalke opened pull request #18247: test: Wait for both veracks in add_p2p_connection (master...2003-qaMininodeVerackRace) https://github.com/bitcoin/bitcoin/pull/18247
< bitcoin-git> [bitcoin] MarcoFalke opened pull request #18249: test: Bump timeouts to accomodate really slow disks (master...2003-testTimeout) https://github.com/bitcoin/bitcoin/pull/18249