🤔 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.

💬 furszy commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#discussion_r1290848071)
Isn't this logged in the caller as well?

💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1674171648)
@luke-jr there was discussion [here](https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173964808) regarding whether to keep the RPC hidden/public.

it was kept as a hidden RPC because:
1. a normal user wouldn't be interested in the new/tried table breakdown of addresses.
2. `-generate CLI` currently [uses](https://github.com/bitcoin/bitcoin/blob/467fa8943801911c233cb96d45282b1de10bfa90/src/bitcoin-cli.cpp#L1178) a [hidden RPC](https://github.com/bitcoin/bitcoin/blob/467fa8943801911
...

📝 stratospher converted_to_draft a pull request: "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988)
implements https://github.com/bitcoin/bitcoin/issues/26907. built on top of https://github.com/bitcoin/bitcoin/pull/27511.

Rework of `-addrinfo` CLI is done using `getaddrmaninfo` RPC proposed in https://github.com/bitcoin/bitcoin/pull/27511. This would be useful for users who want to know the total number of addresses the node knows about and can make connections to.

Currently, `-addrinfo` returns total number of addresses the node knows about after filtering them for quality + recency
...

💬 Jones098 commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1290864136)
Thanks all

🤔 ariard reviewed a pull request: "validation: avoid mempool eviction mid-package evaluation"
(https://github.com/bitcoin/bitcoin/pull/28251#pullrequestreview-1572955374)
Note the code path affected by the bug is under the `submitpackage` RPC, which is clearly indicated as an experimental RPC with an unstable interface.

More test coverage sounds good.