💬 instagibbs commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548247127)
If we're not shooting for a backport for 25.x, I don't think the time difference is meaningful? This PR is adding stuff that will directly be removed and is sort of a distraction for reviewers themselves.

💬 ryanofsky commented on pull request "init: verify blocks data existence only once for all the indexers":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1548248095)
re: https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1544648748

> We could also move the entire `hasDataFromTipDown` from the interface to somewhere else in this way. So there is no `CBlockIndex` dependency in the interface neither here nor in the #24230 intermediate commits.

Yes I don't think it makes sense to expose that method as part of the `Chain` interface, and even if it did make sense, it wouldn't make sense to put the implementation in the `ChainImpl` class, because the
...

💬 Sjors commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1194135762)
Note to others: we may send outbound peers 7 * 2.5 * 60 * 2 ~= 2100 transaction ids per 2 minutes. Depending on random variation it can be more. `UNCONDITIONAL_RELAY_DELAY = 2min;` is the period of time that we have to remember what we sent to a specific peer. For privacy reason we don't want to reveal these transactions to _other_ (spy) peers yet. We do this tracking with a bloom filter `m_recently_announced_invs` that has a 1 in a million chance of messing up. I forgot if it's false positive (
...

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1194151828)
Thank you for the time you took for these detailed comments. Fleshing this out to a more proper kernel interface sounds good to me. How about moving `ValidationNotifications` to the `KernelContext` instead? Or does that conflict with the vision you have for getting rid of `uiInterface` global?

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1194168060)
> Thank you for the time you took for these detailed comments. Fleshing this out to a more proper kernel interface sounds good to me. How about moving `ValidationNotifications` to the `KernelContext` instead? Or does that conflict with the vision you have for getting rid of `uiInterface` global?

Hmm, It doesn't make sense to me that an object which is supposed to receive notifications from the kernel would be owned by the kernel. Outside code that wants to receive notifications should be able
...

💬 joostjager commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548293535)
Yes, backport was the idea 😎 😃

🤔 instagibbs reviewed a pull request: "validate package transactions with their in-package ancestor sets"
(https://github.com/bitcoin/bitcoin/pull/26711#pullrequestreview-1427036930)
looking good

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1194144678)
even more sensible than my suggestion :+1: no issues running so far

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1194150250)
ASCII :gem: needs connection between grandparent and child

💬 instagibbs commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548332581)
fwiw I'm ok getting this in for backport. This is most cruft at the RPC layer that can be reverted on the next PR.

💬 codexnakamoto commented on pull request "build: Drop support for g++-8":
(https://github.com/bitcoin/bitcoin/pull/27662#issuecomment-1548338939)
Concept ACK - this is logical and seems uncontentious.

👍 pinheadmz approved a pull request: "init: Error if ignored bitcoin.conf file is found"
(https://github.com/bitcoin/bitcoin/pull/27302#pullrequestreview-1426880326)
ACK 0319de5cbedd1a8f8766cfec61151c58b3fb27ef

Code review and run bitcoind and -qt with various conf file configurations to see errors and warnings. A few style nits below, can be ignored (especially the release note since that will get reviewed again upon actual release)

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 0319de5cbedd1a8f8766cfec61151c58b3fb27ef
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJ
...

💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194133266)
nit: indent should be +2 more spaces

💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194151784)
I think this will unnecessarily repeat the entire path, could maybe drop `from %4$s`, I got this message just now:


> Data directory "/Users/matthewzipkin/Library/Application Support/Bitcoin/whoa" contains a "bitcoin.conf" file which is ignored, because a different configuration file "/Users/matthewzipkin/Library/Application Support/Bitcoin/bitcoin.conf" from data directory "/Users/matthewzipkin/Library/Application Support/Bitcoin" is being used instead.

💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194040447)
```suggestion
- `bitcoind` and `bitcoin-qt` will now raise an error on startup if a datadir that is being used contains a bitcoin.conf file that will be ignored, which can happen when a datadir= line is used in a bitcoin.conf file. The error message is just a diagnostic intended to prevent accidental misconfiguration, and it can be disabled to restore the previous behavior of using the datadir while ignoring the bitcoin.conf contained in it.
```

💬 furszy commented on issue ""Create Unsigned" should not show the message: "The amount exceeds you balance" without suggesting alternatives":
(https://github.com/bitcoin/bitcoin/issues/27659#issuecomment-1548438693)
> > So, probably the `importdescriptors` RPC command call was done with `timestamp=now` and no rescan was performed.
>
> I confirm that there was the `importdescriptors` RPC command call done with `timestamp=now` and no rescan was performed.

Based on that, maybe close this issue?

I think that adding the "Do you want to start rescanning Bitcoin blockchain in order to update the balance?" inside the sending process is an overkill (it will affect other scenarios and not only this one), lea
...

📝 brunoerg opened a pull request: "docs: fix spelling errors"
(https://github.com/bitcoin/bitcoin/pull/27664)
Add `lief` to `spelling.ignore-words` since it's the name of a Python lib and fix spelling error in `interface_usdt_utxocache` (s/eariler/earlier)

💬 pinheadmz commented on pull request "test: add unit test coverage for Python ECDSA implementation":
(https://github.com/bitcoin/bitcoin/pull/27653#discussion_r1194287587)
IIUC we don't assert that the keys we expect to be invalid are marked correctly. Seems like we mine as well ensure that `generate_privkey()` is always valid but certain edge cases are not (`0`, `SECP256K1_ORDER`, and `2**256 - 1`)

🤔 pinheadmz reviewed a pull request: "test: add unit test coverage for Python ECDSA implementation"
(https://github.com/bitcoin/bitcoin/pull/27653#pullrequestreview-1427256339)
concept ACK

I also think it would be cool to see a set of ECDSA test vectors here. We already have a set of BIP340 vectors tested both in python and in the `key_tests.cpp` unit test, assuring that both implementations handle Schnorr (at least those cases) identically.

💬 sdaftuar commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1548505174)
> My understanding is that `REPLACED` would also need to be provided to peers: [#27625 (comment)](https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544171255). Otherwise there may be another round trip if the miner mines a previous version of a recently replaced transaction, which is in `mapRelay` and `vExtraTxnForCompact`, but not the mempool (and not yet in a block, because it may be in-flight)?

I think it's better that we not relay transactions that are `REPLACED` -- note that we
...