< achow101>
github responded to me about 11857 and they told me that they're looking into it. apparently some other PRs in other projects are seeing similar issues
< promag>
wumpus: not sure if I understand your argument, currently CWalletRef is narrowly used. Depending on the case it can be acceptable to use either reference, raw pointer or shared pointer. When adding shared pointer I would like to review all places where it makes sense to use it. If we had CWalletRef everywhere then that would obscure that evaluation.
<@wumpus>
promag: it's usually preferable to keep diffs minimal, without a type such as CWalletRef it means every usage has to be changed if the underlying type changes, the whole reason typedefs exist...
<@wumpus>
most users of CWalletRef won't care how it works internally
<@wumpus>
they just want to refer to a wallet
< promag>
but my point is that not all CWallet* should be std::shared_ptr
<@wumpus>
maybe not, i still think an abstract reference type makes sense
< promag>
I think that makes sense if CWalletRef was used where it makes sense to switch to shared pointer, but that's not the case
<@wumpus>
yes I agree they don't all need to be replaced
<@wumpus>
what annoyed me is the back and forth, one refactor introduces a certain type then the next one removes it again
< promag>
even if I keep CWalletRef, switching to std::shared_ptr will cause a (not that) large diff
<@wumpus>
so let's just get on one page first
< promag>
typedef aside, the code base must be refactored to shared pointers
<@wumpus>
my point was not 'this is a bad idea' but 'this needs discussion with the people involved'
<@wumpus>
if there is general agreement to undo the CWalletRef type again, that's ok with me
< promag>
wumpus: fair enough, I can revert it too. I should have predicted this :D
<@wumpus>
though it makes me wonder what reasoning introduced it and what changed since
< bitcoin-git>
[bitcoin] qshuai opened pull request #13042: Calculated nBits will be replaced by the following GetNextWorkRequire… (master...master) https://github.com/bitcoin/bitcoin/pull/13042
< cfields>
wumpus: with our powers combined, we've finally arrived at &= :)
< cfields>
no clue what I was thinking there.
< BlueMatt>
sipa: whats your current feeling on caching witness hashes/the sighash components somewhere on mempool acceptance?
< sipa>
BlueMatt: caching sure, but not in the same class that is used for everything
< BlueMatt>
yea, ok, fair
< BlueMatt>
I thought that was rejected on the first go around :/
< BlueMatt>
wumpus: re: #12998: I'm open to doing whatever to fix it, that was just the simplest thing (fix it for the narrow case when those functions are #define'd by the compiler's headers). cfields suggested moving those functions to _int, but I'm not sure making a downstream project's build scripts easier is worth the refactor for that, but up to y'all
< gribble>
https://github.com/bitcoin/bitcoin/issues/12998 | Default to defining endian-conversion DECLs in compat w/o config by TheBlueMatt · Pull Request #12998 · bitcoin/bitcoin · GitHub
< BlueMatt>
I just wanna figure out if I should just apply that patch downstream or what
< jcohen>
i have a half completed change somewhere that makes proper types out of TxId / WTxId instead of using uint256 somewhere - i can clean up if there's to be more pervasive use of witness ids - doesn't do anything other than readability / compiler safety
< luke-jr>
wumpus: CWalletRef was intentionally introduced such that it can be replaced with a shared_ptr painlessly
< luke-jr>
I probably still have a branch with that change
< bitcoin-git>
[bitcoin] instagibbs opened pull request #13045: [p2p] getblock for 1-block reorgs in response to compact block message (master...cmpcttie) https://github.com/bitcoin/bitcoin/pull/13045
< cfields>
BlueMatt: I think it's fine as-is. But if you want to take it further, I think it'd be pretty trivial to switch to _int.
< jnewbery>
13017 doesn't make replacing the raw pointers with shared_ptrs and easier or more difficult, so I don't think it really needs too much discussion.
< jimpo>
Can I get a look on #12647? It's pretty trivial.
< promag>
luke-jr: mind checking for the shared pointer branch, I would like to see it? There are lot of places where CWallet* must be changed to CWalletRef or std::shared_ptr<CWallet>, and IMO there are some places where CWalletRef should be CWallet* IMO. I've dropped CWalletRef so that the PR introducing shared_ptr would do it in the whole source.