🤔 jonatack reviewed a pull request: "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain"
(https://github.com/bitcoin/bitcoin/pull/28157#pullrequestreview-1572801253)
Concept ACK

💬 jonatack commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1290730731)
I'm not sure why, and maybe it's an issue with my local environment as the CI is green, but this test passes for me in the previous push 9bd96327232ac9fbb7f9cb0a0545a4410bdd4964 but not with the latest push ef19d52e25407df1a56a76038dd4c95e03e75250; only the signet case passes while the main and test cases raise the following:

```
AssertionError: [node 0] Expected message "Error: acceptstalefeeestimates is not supported on test chain." does not fully match stderr:
"Error: Unable to start HTT
...

💬 jonatack commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1290746042)
It turns out there isn't a difference between the two pushes. The issue comes from having bitcoind already running on mainnet, testnet, or signet; if yes, the test fails with the message I described.

💬 theuni commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290765007)
Woohoo! I'll PR that commit.

Thanks again for testing, glad to see it's working on macOS (and arm64 even).

💬 ajtowns commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1290764018)
Contrary to its comment, this is only used when validating mempool transactions, so is better placed in policy.h

🤔 ajtowns reviewed a pull request: "Break up script/standard.{h/cpp}"
(https://github.com/bitcoin/bitcoin/pull/28244#pullrequestreview-1572845033)
Approach ACK

💬 ajtowns commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1290764821)
What would be const? Returning a const object (rather than a const reference/pointer) is [largely meaningless](https://stackoverflow.com/questions/8716330/purpose-of-returning-by-const-value).

💬 ajtowns commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1290782479)
Maybe fixup the comment on pubkey.h here too

💬 ajtowns commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1290765177)
Seems strange for this to be a reference (and rely on lifetime extension)?

💬 ajtowns commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#discussion_r1290785301)
`(i == 1) == IsStandardTx(..)` would probably be clearer then?

💬 ajtowns commented on pull request "Libstandardness (edition 2023)":
(https://github.com/bitcoin/bitcoin/pull/28220#issuecomment-1674083758)
> This PR proposes to introduce a new interface to allow applications and second layers protocols to verify that their unconfirmed and non-propagated transactions are valid under Bitcoin Core transaction relay policy.

Better to use the `testmempoolaccept` rpc for this than introduce a new api: mempool policy is a per-node decision, so having an actual node to ask is fairly natural. If you have a particular expectation on what "economically rational" nodes do, then configure that node be an "e
...

💬 ajtowns commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1290819834)
Should we add an rpc to allow querying the orphanage?

⚠️ netpipe opened an issue: "easy loading custom blockchains"
(https://github.com/bitcoin/bitcoin/issues/28255)
### Please describe the feature you'd like to see added.

running custom coins(genesis)/configs could be easier to maintain if the source code was built for it rather than forking from it with no idea how to update the forks.

### Is your feature related to a problem, if so please describe it.

once forked and modified its hard to maintain (stay uptodate) to existing bitcoin source tree automatically

### Describe the solution you'd like

put custom coin genesis loading/generation into the main
...

🤔 ajtowns reviewed a pull request: "test: tx orphan handling"
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1572916458)
ACK

I think we should probably prefer to test behaviours that are intentional/desirable, rather than behaviours that just happen to be how it got implemented. It's not immediately clear to me which class the things being tested here fall into.

💬 ajtowns commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1290824888)
Maybe name this something that's a bit more clearly related to mocktime (`bumpmocktime`?) and add it to `BitcoinTestFramework` ?

💬 ajtowns commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1290823896)
Might be better to be clearer about the different behaviours you're testing for, rather than how you're triggering those behaviours? eg:

* Orphan handling when parent is known to be invalid
* Orphan handling when segwit parent may be retried with alternate witness data

💬 ajtowns commented on pull request "[no merge, meta] refactor: net/net processing split":
(https://github.com/bitcoin/bitcoin/pull/28252#issuecomment-1674134771)
> Most of the API simply shifts from `CConnman::DoX(CNode*)` to `CConnman::DoX(NodeId)`

That seems undesirable to me? Where possible, acting directly on the node via a pointer/reference is much better than taking a global lock, doing a lookup, and then acting.

> * We can't test PeerManager in isolation if it is not the owner of its own state.

I think it's a mistake trying to have many different owners of "per-node" state -- that increases the lookups, locks and coordination required whe
...

💬 furszy commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#discussion_r1290847697)
This will trigger two error dialogs in the GUI, one after the other.
Maybe better to concatenate the errors and call `InitError()` only once?

💬 furszy commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#discussion_r1290782048)
is this include needed?

💬 furszy commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#discussion_r1290849311)
Didn't know that this was possible, pretty nice.