👍 instagibbs approved a pull request: "test: properly check for per-tx sigops limit"
(https://github.com/bitcoin/bitcoin/pull/32533#pullrequestreview-2861227732)
crACK 7bc64a8859c3644fdf2eeff59f72a778ae60ea3f
just nits you can ignore
(https://github.com/bitcoin/bitcoin/pull/32533#pullrequestreview-2861227732)
crACK 7bc64a8859c3644fdf2eeff59f72a778ae60ea3f
just nits you can ignore
💬 fanquake commented on pull request "cmake: Remove `ENABLE_{SSE41,AVX2,X86_SHANI,ARM_SHANI}` from `bitcoin-build-config.h`":
(https://github.com/bitcoin/bitcoin/pull/32551#issuecomment-2901333121)
Backported to 29.x in #32589.
(https://github.com/bitcoin/bitcoin/pull/32551#issuecomment-2901333121)
Backported to 29.x in #32589.
🚀 glozow merged a pull request: "test: properly check for per-tx sigops limit"
(https://github.com/bitcoin/bitcoin/pull/32533)
(https://github.com/bitcoin/bitcoin/pull/32533)
💬 pinheadmz commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2901371140)
Approach ACK fa53098472 (still reviewing code changes around blocktip notifications)
With fresh brain this morning I think this is the best possible approach if we make any change at all. The regtest stuff I observed is expected behavior - if there's no block for 2 days then yes you are 288 blocks behind and progress should estimate that. The new chainparams define regtest as having an expected 0.001 tx/s which is approximately equal to one (coinbase) tx every ten minutes.
Example catchi
...
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2901371140)
Approach ACK fa53098472 (still reviewing code changes around blocktip notifications)
With fresh brain this morning I think this is the best possible approach if we make any change at all. The regtest stuff I observed is expected behavior - if there's no block for 2 days then yes you are 288 blocks behind and progress should estimate that. The new chainparams define regtest as having an expected 0.001 tx/s which is approximately equal to one (coinbase) tx every ten minutes.
Example catchi
...
💬 maflcko commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2102669940)
Just to avoid to take the lock again. You suggestion is also fine. Happy to push it, just let me know.
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2102669940)
Just to avoid to take the lock again. You suggestion is also fine. Happy to push it, just let me know.
💬 maflcko commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2102672146)
the lock is recursive and all callers already held it, except for one, which is handled in the next commit: https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2100887156
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2102672146)
the lock is recursive and all callers already held it, except for one, which is handled in the next commit: https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2100887156
💬 pinheadmz commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2102678855)
Ah thanks, good justification, thats good 👍
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2102678855)
Ah thanks, good justification, thats good 👍
💬 hebasto commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2901391171)
> > I've restored the `shell` option in the `subprocess` API, and employed it on non-Windows systems.
>
> Hmm the shell support implemented in [eb16060](https://github.com/bitcoin/bitcoin/commit/eb160602a50bebcca3f53cdae741e1732738d51a) actually seems somewhat broken as it is still splitting and joining instead of preserving the original string passed to Popen() instead of just passing it unaltered to execvp as I was suggesting.
Thanks! Implemented as you suggested.
(no further clean up
...
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2901391171)
> > I've restored the `shell` option in the `subprocess` API, and employed it on non-Windows systems.
>
> Hmm the shell support implemented in [eb16060](https://github.com/bitcoin/bitcoin/commit/eb160602a50bebcca3f53cdae741e1732738d51a) actually seems somewhat broken as it is still splitting and joining instead of preserving the original string passed to Popen() instead of just passing it unaltered to execvp as I was suggesting.
Thanks! Implemented as you suggested.
(no further clean up
...
⚠️ fanquake opened an issue: "seeds: seed.bitcoin.jonasschnelli.ch not returning results"
(https://github.com/bitcoin/bitcoin/issues/32590)
Also shown by [check-dnsseeds.py](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/check-dnsseeds.py):
```bash
./check-dnsseeds.py
* mainnet
OK seed.bitcoin.sipa.be (39 results)
OK dnsseed.bluematt.me (31 results)
OK dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us (35 results)
OK seed.bitcoinstats.com (24 results)
FAIL seed.bitcoin.jonasschnelli.ch
OK seed.btc.petertodd.net (37 results)
OK seed.bitcoin.sprovoost.nl (36 results)
OK dnsseed.emzy.de (40 results)
OK see
...
(https://github.com/bitcoin/bitcoin/issues/32590)
Also shown by [check-dnsseeds.py](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/check-dnsseeds.py):
```bash
./check-dnsseeds.py
* mainnet
OK seed.bitcoin.sipa.be (39 results)
OK dnsseed.bluematt.me (31 results)
OK dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us (35 results)
OK seed.bitcoinstats.com (24 results)
FAIL seed.bitcoin.jonasschnelli.ch
OK seed.btc.petertodd.net (37 results)
OK seed.bitcoin.sprovoost.nl (36 results)
OK dnsseed.emzy.de (40 results)
OK see
...
💬 hebasto commented on pull request "depends: hard-code necessary c(xx)flags rather than setting them per-host":
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-2901416521)
My Guix build:
```
aarch64
7a5858e3364998675e06a2bec330a6fb9f619613a9228a568b5c98cba873460f guix-build-0275825f7ace/output/aarch64-linux-gnu/SHA256SUMS.part
a8b08f5f246ca97ae26bf9142355797f82d3f1e7eb2ffbc1b7d78f35c4c0cd4d guix-build-0275825f7ace/output/aarch64-linux-gnu/bitcoin-0275825f7ace-aarch64-linux-gnu-debug.tar.gz
92cbdec0549173a3e3a90ac81ad2dac98d9758a76efe12a6d205cc1265f9f701 guix-build-0275825f7ace/output/aarch64-linux-gnu/bitcoin-0275825f7ace-aarch64-linux-gnu.tar.gz
c7915b68
...
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-2901416521)
My Guix build:
```
aarch64
7a5858e3364998675e06a2bec330a6fb9f619613a9228a568b5c98cba873460f guix-build-0275825f7ace/output/aarch64-linux-gnu/SHA256SUMS.part
a8b08f5f246ca97ae26bf9142355797f82d3f1e7eb2ffbc1b7d78f35c4c0cd4d guix-build-0275825f7ace/output/aarch64-linux-gnu/bitcoin-0275825f7ace-aarch64-linux-gnu-debug.tar.gz
92cbdec0549173a3e3a90ac81ad2dac98d9758a76efe12a6d205cc1265f9f701 guix-build-0275825f7ace/output/aarch64-linux-gnu/bitcoin-0275825f7ace-aarch64-linux-gnu.tar.gz
c7915b68
...
🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: add TxGraph reorg functionality"
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2860445285)
Code review up to 412a9e0e69ccf461554660b7c622b768cfb8ccb3
Also did some fuzzing and mutate the new code added and tests, craches occur as expected.
> If a transaction is encountered whose addition would violate the limit, it is removed, together with all its descendants.
In real-world scenarios it is unlikely we encounter those transactions that will exceed the cluster count limit, however when it occur I assume that the newly connected block will have similar transaction with the reor
...
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2860445285)
Code review up to 412a9e0e69ccf461554660b7c622b768cfb8ccb3
Also did some fuzzing and mutate the new code added and tests, craches occur as expected.
> If a transaction is encountered whose addition would violate the limit, it is removed, together with all its descendants.
In real-world scenarios it is unlikely we encounter those transactions that will exceed the cluster count limit, however when it occur I assume that the newly connected block will have similar transaction with the reor
...
💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102062249)
This method's name aligns with its functionality.
However with the addition of this commit I think there is ambiguity currently between the size oversize, the count oversize and both. This makes it a bit confusing to follow.
maybe separate the two.
oversize should be for size.
overlimit or something like it for both
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102062249)
This method's name aligns with its functionality.
However with the addition of this commit I think there is ambiguity currently between the size oversize, the count oversize and both. This makes it a bit confusing to follow.
maybe separate the two.
oversize should be for size.
overlimit or something like it for both
💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102457782)
assert that we trim when oversized ?
```suggestion
assert(!removed.empty());
auto removed_set = top_sim.MakeSet(removed);
```
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102457782)
assert that we trim when oversized ?
```suggestion
assert(!removed.empty());
auto removed_set = top_sim.MakeSet(removed);
```
💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102277142)
if there is nothing in the cluster? why should it be marked as needs_split_acceptable?
Also I see no crashes after removing the new or condition.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102277142)
if there is nothing in the cluster? why should it be marked as needs_split_acceptable?
Also I see no crashes after removing the new or condition.
💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102494391)
check both ways
```suggestion
}
// No element exist in set and cluster is oversized
if (is_oversized && !set.Overlaps(component)){
return false;
}
```
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102494391)
check both ways
```suggestion
}
// No element exist in set and cluster is oversized
if (is_oversized && !set.Overlaps(component)){
return false;
}
```
💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102571828)
Indicate that it should not be called when their is an instance of block builder.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102571828)
Indicate that it should not be called when their is an instance of block builder.
📝 instagibbs opened a pull request: "test: fix and augment block tests of invalid_txs"
(https://github.com/bitcoin/bitcoin/pull/32591)
Issue discovered while reviewing https://github.com/bitcoin/bitcoin/pull/32533
(https://github.com/bitcoin/bitcoin/pull/32591)
Issue discovered while reviewing https://github.com/bitcoin/bitcoin/pull/32533
🤔 rkrux reviewed a pull request: "wallet: Remove watchonly behavior and isminetypes"
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-2860781255)
Initial code review f3ab751206d4c3db8696ee541f9d253ce162f295
I'm in favour of this clean up: maintaining watch-only per output is cumbersome in the code and seems complicated to me from a user's point of view as well, great that the descriptor wallets provide watch-only behaviour on the wallet level.
The CI error seems unrelated as it's a timeout in the external signer test and it's happening in other PRs as well.
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-2860781255)
Initial code review f3ab751206d4c3db8696ee541f9d253ce162f295
I'm in favour of this clean up: maintaining watch-only per output is cumbersome in the code and seems complicated to me from a user's point of view as well, great that the descriptor wallets provide watch-only behaviour on the wallet level.
The CI error seems unrelated as it's a timeout in the external signer test and it's happening in other PRs as well.
💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102275537)
IMO this was quite complicated to reason about and have in the wallet.
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102275537)
IMO this was quite complicated to reason about and have in the wallet.
💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102275063)
Nit:
```diff
- "and relation to the wallet (ismine)."},
+ "and relation to the wallet."},
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102275063)
Nit:
```diff
- "and relation to the wallet (ismine)."},
+ "and relation to the wallet."},