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

💬 ariard commented on pull request "validation: avoid mempool eviction mid-package evaluation":
(https://github.com/bitcoin/bitcoin/pull/28251#discussion_r1290865427)
I think this could document more where the “do not LimitMempool” is applied, i.e in `Finalize()` L1123 which is itself called by `AcceptSingleTransaction` and the source of the bug. `LimitMempoolSize()` is called few times in validation.

💬 ariard commented on pull request "validation: avoid mempool eviction mid-package evaluation":
(https://github.com/bitcoin/bitcoin/pull/28251#discussion_r1290866687)
I think this check doesn’t have test coverage as tx are individually validated in `AcceptPackage`.

💬 ariard commented on pull request "validation: avoid mempool eviction mid-package evaluation":
(https://github.com/bitcoin/bitcoin/pull/28251#discussion_r1290857304)
The “disabled RBF” can sound as confusing. There is no nsequence opted-out of the transaction candidate targeted by a replacement, it sounds rather to mean the `m_allow_replacement` of `ATMPArgs` which is true for `SingleInPackageAccept` and false for `PackageChildWithParents`.

💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1290884007)
done. added some more detail to the part about new table.

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

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