💬 furszy commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1994294882)
tiny nit: could provide a less aggressive `check_interval`. Like 0.1.
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1994294882)
tiny nit: could provide a less aggressive `check_interval`. Like 0.1.
📝 glozow opened a pull request: "[29.x] backports and rc2"
(https://github.com/bitcoin/bitcoin/pull/32062)
Placeholder, collecting backports + anything else for rc2
- #32049
(https://github.com/bitcoin/bitcoin/pull/32062)
Placeholder, collecting backports + anything else for rc2
- #32049
💬 darosior commented on pull request "test: avoid treating hash results as integers":
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722704531)
The concept of this PR is appealing but i don't think it's true that we don't usually rely on hashes being numbers. I think more often than not we do, as we have a bunch of ordered indexes keyed by hashes. A good example of this is the mempool's `mapTx`:
https://github.com/bitcoin/bitcoin/blob/5c2f04413e49545cbefcd232f9c1b6bd08688899/src/txmempool.h#L245-L250
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722704531)
The concept of this PR is appealing but i don't think it's true that we don't usually rely on hashes being numbers. I think more often than not we do, as we have a bunch of ordered indexes keyed by hashes. A good example of this is the mempool's `mapTx`:
https://github.com/bitcoin/bitcoin/blob/5c2f04413e49545cbefcd232f9c1b6bd08688899/src/txmempool.h#L245-L250
👍 TheCharlatan approved a pull request: "test: Check datadir cleanup after assumeutxo was successful"
(https://github.com/bitcoin/bitcoin/pull/32033#pullrequestreview-2683379972)
ACK dfd0a76fc1b46ddd1f4dab388ec2ca7fc3f76a2b
(https://github.com/bitcoin/bitcoin/pull/32033#pullrequestreview-2683379972)
ACK dfd0a76fc1b46ddd1f4dab388ec2ca7fc3f76a2b
💬 TheCharlatan commented on pull request "test: Check datadir cleanup after assumeutxo was successful":
(https://github.com/bitcoin/bitcoin/pull/32033#discussion_r1994314303)
Nit: Might it be worthwhile to also check existence beforehand to guard against naming changes, or the behaviour of the indexed nodes changing?
```diff
diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index 89cf703fce..2166c4505d 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -645,0 +646,2 @@ class AssumeutxoTest(BitcoinTestFramework):
+ if i != 0:
+ assert (n.datadir_pa
...
(https://github.com/bitcoin/bitcoin/pull/32033#discussion_r1994314303)
Nit: Might it be worthwhile to also check existence beforehand to guard against naming changes, or the behaviour of the indexed nodes changing?
```diff
diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index 89cf703fce..2166c4505d 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -645,0 +646,2 @@ class AssumeutxoTest(BitcoinTestFramework):
+ if i != 0:
+ assert (n.datadir_pa
...
💬 theuni commented on pull request "build: align debugging flags to `-O0`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2722723839)
@fanquake Can you describe why you think it's important that they align? I think they're 2 different concerns, really. It seems like it'd be a pretty rare circumstance where we'd need depends compiled with max gdb debugability, but pretty common that we'd want Core built that way.
Aligning depends and Core builds both at `-Og` seemed reasonable to me, but @achow101 has a compelling argument against that. I'm not so excited about both at `-O0` because that's basically _only_ useful for debuggi
...
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2722723839)
@fanquake Can you describe why you think it's important that they align? I think they're 2 different concerns, really. It seems like it'd be a pretty rare circumstance where we'd need depends compiled with max gdb debugability, but pretty common that we'd want Core built that way.
Aligning depends and Core builds both at `-Og` seemed reasonable to me, but @achow101 has a compelling argument against that. I'm not so excited about both at `-O0` because that's basically _only_ useful for debuggi
...
💬 furszy commented on pull request "wallet: refactor: various master key encryption cleanups":
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1994324573)
Not related to this PR but if `WriteMasterKey()` fails then this will still succeed when it shouldn't.
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1994324573)
Not related to this PR but if `WriteMasterKey()` fails then this will still succeed when it shouldn't.
💬 furszy commented on pull request "wallet: refactor: various master key encryption cleanups":
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1994315843)
In 62c209f50d9c33:
What about inlining this into the (un)serialization method?
If it is not used, we don't need to keep the field in memory.
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1994315843)
In 62c209f50d9c33:
What about inlining this into the (un)serialization method?
If it is not used, we don't need to keep the field in memory.
💬 furszy commented on pull request "wallet: refactor: various master key encryption cleanups":
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1994325143)
Same as above; not related to this PR but if `WriteMasterKey()` fails then this will still succeed when it shouldn't.
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1994325143)
Same as above; not related to this PR but if `WriteMasterKey()` fails then this will still succeed when it shouldn't.
💬 davidgumberg commented on pull request "[29.x] backports and rc2":
(https://github.com/bitcoin/bitcoin/pull/32062#issuecomment-2722748021)
lgtm ACK https://github.com/bitcoin/bitcoin/pull/32062/commits/188ec87589476355a4280f8a561d954b9fbbb7af
(https://github.com/bitcoin/bitcoin/pull/32062#issuecomment-2722748021)
lgtm ACK https://github.com/bitcoin/bitcoin/pull/32062/commits/188ec87589476355a4280f8a561d954b9fbbb7af
💬 ryanofsky commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2722752125)
Code review d700b14491a27a6f4f9069dde86fe914784346ca
This approach seems reaonable but maybe it would be good to have a more general solution to providing buffered reads and writes instead of having these ad-hoc `std::vector<uint8_t>` / `SpanReader` / `DataStream` / `write_large` calls.
Ideally instead of:
```c++
std::vector<uint8_t> mem(blk_size);
filein >> Span{mem};
SpanReader(mem) >> TX_WITH_WITNESS(block);
// ...
fileout.write_large(DataStream(block_size) << TX_WITH_WITNESS(bl
...
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2722752125)
Code review d700b14491a27a6f4f9069dde86fe914784346ca
This approach seems reaonable but maybe it would be good to have a more general solution to providing buffered reads and writes instead of having these ad-hoc `std::vector<uint8_t>` / `SpanReader` / `DataStream` / `write_large` calls.
Ideally instead of:
```c++
std::vector<uint8_t> mem(blk_size);
filein >> Span{mem};
SpanReader(mem) >> TX_WITH_WITNESS(block);
// ...
fileout.write_large(DataStream(block_size) << TX_WITH_WITNESS(bl
...
💬 davidgumberg commented on pull request "test: Add support for mainnet addresses in address_to_scriptpubkey":
(https://github.com/bitcoin/bitcoin/pull/32060#discussion_r1994341807)
nit: There are other address types to support so the todo comment should not be removed
(https://github.com/bitcoin/bitcoin/pull/32060#discussion_r1994341807)
nit: There are other address types to support so the todo comment should not be removed
💬 davidgumberg commented on pull request "test: Add support for mainnet addresses in address_to_scriptpubkey":
(https://github.com/bitcoin/bitcoin/pull/32060#discussion_r1994347199)
```suggestion
if version == 111 or version == 0: # testnet or mainnet pubkey hash
return keyhash_to_p2pkh_script(payload)
elif version == 196 or version == 5: # testnet or mainnet script hash
return scripthash_to_p2sh_script(payload)
```
(https://github.com/bitcoin/bitcoin/pull/32060#discussion_r1994347199)
```suggestion
if version == 111 or version == 0: # testnet or mainnet pubkey hash
return keyhash_to_p2pkh_script(payload)
elif version == 196 or version == 5: # testnet or mainnet script hash
return scripthash_to_p2sh_script(payload)
```
💬 mzumsande commented on issue "intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2722773418)
> Looks like the test may overall intermittently fail, because it assumes the tx delay isn't exponentially distributed, but has a constant delay, sufficient for the test to pass.
I think that is not the problem here, because the delay for `requesting` txs is actually constant and not randomized / exponentially distributed.
> Possibly the getdata was split into two, so the check didn't pick it up, because it assumes it to happen in one msg
Yes, this is what's happening in the failed runs. I th
...
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2722773418)
> Looks like the test may overall intermittently fail, because it assumes the tx delay isn't exponentially distributed, but has a constant delay, sufficient for the test to pass.
I think that is not the problem here, because the delay for `requesting` txs is actually constant and not randomized / exponentially distributed.
> Possibly the getdata was split into two, so the check didn't pick it up, because it assumes it to happen in one msg
Yes, this is what's happening in the failed runs. I th
...
📝 mzumsande opened a pull request: "test: fix intermittent failure in p2p_orphan_handling.py"
(https://github.com/bitcoin/bitcoin/pull/32063)
If the mocktime is bumped before the node has successfully disconnected the peer, the requests for both parents could be spread over two GETDATAS: The first time `GetRequestsToSend` is invoked it would only request one tx from peer2, because the other one would only be available after peer1 was disconnected and its outstanding txrequest cleared.
So two GETDATAs would be sent, which would make the test fail.
Fixes #31700
(https://github.com/bitcoin/bitcoin/pull/32063)
If the mocktime is bumped before the node has successfully disconnected the peer, the requests for both parents could be spread over two GETDATAS: The first time `GetRequestsToSend` is invoked it would only request one tx from peer2, because the other one would only be available after peer1 was disconnected and its outstanding txrequest cleared.
So two GETDATAs would be sent, which would make the test fail.
Fixes #31700
💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2722882679)
I think I agree with all that, and think chainstate objects should just contain information for chainstatemanager to deal with them, not to have "rich" data or application specific data. I also think depending on what you want to do with chainstates, block status flags may provide enough information, but they are not equivalent to the old `Chainstate::m_disabled` field or the new `Chainstate::m_validity` field replacing it, since block flags only tell you about validity back to the last snapshot
...
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2722882679)
I think I agree with all that, and think chainstate objects should just contain information for chainstatemanager to deal with them, not to have "rich" data or application specific data. I also think depending on what you want to do with chainstates, block status flags may provide enough information, but they are not equivalent to the old `Chainstate::m_disabled` field or the new `Chainstate::m_validity` field replacing it, since block flags only tell you about validity back to the last snapshot
...
💬 fjahr commented on issue "Fully validated AssumeUTXO starts revalidating after restart":
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2722961366)
I would still be concept ACK on a PR from @mzumsande if it doesn't turn out to be too complicated. I think James built in a lot of belts and suspenders because initially not everyone was fully on board with the concept and it makes sense to clean this up now if it's just redundant work.
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2722961366)
I would still be concept ACK on a PR from @mzumsande if it doesn't turn out to be too complicated. I think James built in a lot of belts and suspenders because initially not everyone was fully on board with the concept and it makes sense to clean this up now if it's just redundant work.
🤔 pablomartin4btc reviewed a pull request: "qa: clarify and document one assumeutxo test case with malleated snapshot"
(https://github.com/bitcoin/bitcoin/pull/31907#pullrequestreview-2683628002)
post-merge ACK e5ff4e416ecc8a51367022eb8a7a291f8cbc0c65
The updated case looks obviously much clearer now (3rd commit body explains the change very well too) and I'm in favour of the [mentioned](https://github.com/bitcoin/bitcoin/pull/31907#pullrequestreview-2661853159) follow-ups.
(https://github.com/bitcoin/bitcoin/pull/31907#pullrequestreview-2683628002)
post-merge ACK e5ff4e416ecc8a51367022eb8a7a291f8cbc0c65
The updated case looks obviously much clearer now (3rd commit body explains the change very well too) and I'm in favour of the [mentioned](https://github.com/bitcoin/bitcoin/pull/31907#pullrequestreview-2661853159) follow-ups.
🚀 fanquake merged a pull request: "consensus: Remove checkpoints (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31649)
(https://github.com/bitcoin/bitcoin/pull/31649)
💬 achow101 commented on issue "v29.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/32052#issuecomment-2723007926)
#31844 Broke MacOS codesigning and notarization as it adds the manpage to the app, which `signapple` currently cannot cover in its signature. Since we did not have the manpage there before, and I don't think MacOS can even use that manpage, I think it would be reasonable to drop that from the app. I think we can skip codesigning for 29.0rc1.
(https://github.com/bitcoin/bitcoin/issues/32052#issuecomment-2723007926)
#31844 Broke MacOS codesigning and notarization as it adds the manpage to the app, which `signapple` currently cannot cover in its signature. Since we did not have the manpage there before, and I don't think MacOS can even use that manpage, I think it would be reasonable to drop that from the app. I think we can skip codesigning for 29.0rc1.