💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291532701)
The grandparent transaction has a higher feerate, so we only need to bump the parent. We check that if we get the complete chain of unconfirmed transactions, that the resulting feerate is above our target.

💬 TheCharlatan commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1675009460)
Re https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1674826021

> Presumably when de-globalizing, these functions will need the kernel anyway.

I am not sure how we could go about registering our signal handlers without the shutdown global.

> It would document/force the correct creation order

I originally wanted to to introduce this in a separate PR (fixing the order for gui initialization), but I think it makes more sense here, yes.

📝 TheCharlatan converted_to_draft a pull request: "kernel: Run sanity checks on context construction"
(https://github.com/bitcoin/bitcoin/pull/28228)
Moving the kernel sanity check code to kernel context instantiation time ensures that it is impossible to create an invalid kernel context. This is based on a suggestion made in a [comment](https://github.com/bitcoin/bitcoin/pull/25065#discussion_r882182549) by @theuni in the [original pull request](https://github.com/bitcoin/bitcoin/pull/25065) first introducing the `kernel::Context`.

Since this touches the daemon initialization logic, also fix a bug where it would be possible for a terminat
...

💬 theuni commented on pull request "[no merge, meta] refactor: net/net processing split":
(https://github.com/bitcoin/bitcoin/pull/28252#issuecomment-1675023954)
We've debated the CNode* passing/abstracting thing ever since CConnman was merged. This has come up over and over so it's definitely not new. And the big problem is: everyone has their own take. The question of "should we use CNode or CConnman as the interface for network stuff" could be debated forever. I think the only correct answer is "not both arbitrarily", which is what we do now.

I commend @dergoegge for working on this. It's tough and requires picking an approach and sticking to it.

...

💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291548464)
I also updated the `miniminer_1p1c` test to match the transaction numbers to the indices

💬 darosior commented on pull request "fuzz: fix a couple incorrect assertions in the `coins_view` target":
(https://github.com/bitcoin/bitcoin/pull/28215#discussion_r1291552809)
Thanks, done.

💬 darosior commented on pull request "fuzz: fix a couple incorrect assertions in the `coins_view` target":
(https://github.com/bitcoin/bitcoin/pull/28215#issuecomment-1675038081)
> I'd prefer if you summarize what the PR does in the description.

Fair enough, my PR description was sloppy. Done.

> Mostly the fact that these assertions are only correct if we use the default CCoinsView

I would disagree here. While it's true the code is "correct" as in the fuzz target doesn't crash, the logic itself is not correct:
- It's asserting something that is always false, namely that the cache is always in sync with the backend.. Which is antithetical to using a cache in the
...

💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291565216)
That’s a good idea, but I’m gonna leave this, because this matches how we interact with the mempool entries.

💬 pinheadmz commented on issue "RPC `createmultisig` outputs a `sh(addr(...))` descriptor when address_type field is "p2sh-segwit"":
(https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1675072589)
> Will work on the test case and push it. Good eye reporting this @pinheadmz 👌🏼.

props to @Vasu-08 whose work on descriptors in bcoin brought this to my attention ;-)

💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291575150)
Yes, we are doing it manually, because we also compare it against what the mempool thinks it is.

💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1291575841)
I added an `assert(fee);` right before.

📝 Brotcrunsher opened a pull request: "Unified mixture of unnamed namespace and static."
(https://github.com/bitcoin/bitcoin/pull/28259)
Previously we had an unnamed namespace with two functions, one static the other not static. When briefly reading the code this might be slightly misleading and could make the reader think that not both functions are translation unit local, even though they are.

💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1675183615)
Sorry, I still need to squash this, but this should be addressing all the open comments

⚠️ Brotcrunsher opened an issue: "getJsonToken assumes underlying string is null terminated"
(https://github.com/bitcoin/bitcoin/issues/28260)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

When a function requires a `const char *end`, one might assume that the string doesn't need to be null terminated. `getJsonToken` requires this parameter:
```
enum jtokentype getJsonToken(std::string& tokenVal, unsigned int& consumed,
const char *raw, const char *end)
```

However, the function does assume that the underlying memory block is null-terminate
...

💬 brunoerg commented on pull request "test: check backup from `migratewallet` can be successfully restored":
(https://github.com/bitcoin/bitcoin/pull/28257#issuecomment-1675289212)
Thanks, @furszy. Initially, I was thinking about identifying all backup files into the node's directory, run `migratewallet` with them and check the result. However, I agree it's better to go beyond by checking wallet's balance and other stuff prior and post migration.

Force-pushed changing the approach. I also moved it to `test_basic()`.

🤔 furszy reviewed a pull request: "wallet: Allow users to create a wallet that encrypts all database records"
(https://github.com/bitcoin/bitcoin/pull/28142#pullrequestreview-1574384593)
> @furszy That would work. My experience though is that touching anything inside Core's directories can get me in trouble. I once renamed a wallet file and Qt broke. It would only work again if I went into some JSON file to rename the wallet there too..

Well, that is not the case for loading the wallet from an external directory. No need to touch anything on your system. Just run the RPC command.

That being said, if you can trigger any crash by manually changing something on your system,
...

🤔 fjahr reviewed a pull request: "assumeutxo (2)"
(https://github.com/bitcoin/bitcoin/pull/27596#pullrequestreview-1477561344)
Leaving some initial comments and will do some manual testing next.

@jamesob there are several todo comments in the code that sound like you were planning to address them before merge. Are you still planning to address those? Otherwise, could you update the comments so that we know what your plan is for those changes?

💬 fjahr commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291649621)
I am not sure it makes sense to distribute the prune budget between all chainstates equally. Ideally we would sync the background chainstate block by block and throw them away instantly because if we are pruning the likelihood that we keep the bg blocks at the end of IBD is much lower. I'm not sure what the perfect distribution would be but I would shift it more towards the assumed chainstate.

💬 fjahr commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291426426)
Better to be precise and ask them to only recommend deleting `blocks`, `chainstate` and `indexes`. Or alternatively add a reminder that they should backup their wallet before deleting the whole datadir.

💬 fjahr commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1291620032)
Hmm, might be good to update ZMQ and wallet docs as a follow-up that blocks that are connected to the background chain will not be notified...