👍 stickies-v approved a pull request: "rest: bugfix, fix crash error when calling `/deploymentinfo`"
(https://github.com/bitcoin/bitcoin/pull/27853#pullrequestreview-1475484670)
ACK 7d452d826a7056411077b870efc3872bb2fa45e4
(https://github.com/bitcoin/bitcoin/pull/27853#pullrequestreview-1475484670)
ACK 7d452d826a7056411077b870efc3872bb2fa45e4
📝 mzumsande opened a pull request: "test: fix intermittent failure in p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/pull/27864)
Fixes #27860
The problem was that the `inv` for `tx_b` would sometimes be sent out to the inbound peer after the `notfound`, so that the `notfound` wouldn't be the last message anymore and the test fails.
```
node0 2023-06-12T12:48:24.903204Z [msghand] [net.cpp:2856] [PushMessage] [net] sending notfound (73 bytes) peer=1
node0 2023-06-12T12:48:24.903423Z [msghand] [net.cpp:2856] [PushMessage] [net] sending inv (37 bytes) peer=1
```
Fix this by waiting for the `inv` before contin
...
(https://github.com/bitcoin/bitcoin/pull/27864)
Fixes #27860
The problem was that the `inv` for `tx_b` would sometimes be sent out to the inbound peer after the `notfound`, so that the `notfound` wouldn't be the last message anymore and the test fails.
```
node0 2023-06-12T12:48:24.903204Z [msghand] [net.cpp:2856] [PushMessage] [net] sending notfound (73 bytes) peer=1
node0 2023-06-12T12:48:24.903423Z [msghand] [net.cpp:2856] [PushMessage] [net] sending inv (37 bytes) peer=1
```
Fix this by waiting for the `inv` before contin
...
💬 brunoerg commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1227055025)
I think you forgot this. (in case you have to touch the code again)
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1227055025)
I think you forgot this. (in case you have to touch the code again)
💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1227061175)
Hm, I did do it, not sure why it got lost...
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1227061175)
Hm, I did do it, not sure why it got lost...
💬 Xekyo commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1227064407)
The point of this fuzz test is to test the calculation of bump fees. If all outputs of transactions are spent, there are no bump fees to be calculated. With the std::min(2, available_coins.size()) the (2, 5) should also not crash, though. I’ll test that and return it to that if my understanding is substantiated.
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1227064407)
The point of this fuzz test is to test the calculation of bump fees. If all outputs of transactions are spent, there are no bump fees to be calculated. With the std::min(2, available_coins.size()) the (2, 5) should also not crash, though. I’ll test that and return it to that if my understanding is substantiated.
📝 mzumsande converted_to_draft a pull request: "test: fix intermittent failure in p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/pull/27864)
Fixes #27860
The problem was that the `inv` for `tx_b` would sometimes be sent out to the inbound peer after the `notfound`, so that the `notfound` wouldn't be the last message anymore and the test fails.
```
node0 2023-06-12T12:48:24.903204Z [msghand] [net.cpp:2856] [PushMessage] [net] sending notfound (73 bytes) peer=1
node0 2023-06-12T12:48:24.903423Z [msghand] [net.cpp:2856] [PushMessage] [net] sending inv (37 bytes) peer=1
```
Fix this by waiting for the `inv` before contin
...
(https://github.com/bitcoin/bitcoin/pull/27864)
Fixes #27860
The problem was that the `inv` for `tx_b` would sometimes be sent out to the inbound peer after the `notfound`, so that the `notfound` wouldn't be the last message anymore and the test fails.
```
node0 2023-06-12T12:48:24.903204Z [msghand] [net.cpp:2856] [PushMessage] [net] sending notfound (73 bytes) peer=1
node0 2023-06-12T12:48:24.903423Z [msghand] [net.cpp:2856] [PushMessage] [net] sending inv (37 bytes) peer=1
```
Fix this by waiting for the `inv` before contin
...
💬 ryanofsky commented on pull request "wallet: Migrate wallets that are not in a wallet dir":
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1227076942)
Sorry, I overlooked that in my suggestion. But I think this function can do the same thing as [`MakeWalletDatabase`](https://github.com/bitcoin/bitcoin/blob/c92fd638860c5b4477851fb3790bc8068f808d3e/src/wallet/wallet.cpp#L2892), [`VerifyWallets`](https://github.com/bitcoin/bitcoin/blob/c92fd638860c5b4477851fb3790bc8068f808d3e/src/wallet/load.cpp#L79), [`RestoreWallet`](https://github.com/bitcoin/bitcoin/blob/c92fd638860c5b4477851fb3790bc8068f808d3e/src/wallet/wallet.cpp#L446), [`MigrateLegacyToDe
...
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1227076942)
Sorry, I overlooked that in my suggestion. But I think this function can do the same thing as [`MakeWalletDatabase`](https://github.com/bitcoin/bitcoin/blob/c92fd638860c5b4477851fb3790bc8068f808d3e/src/wallet/wallet.cpp#L2892), [`VerifyWallets`](https://github.com/bitcoin/bitcoin/blob/c92fd638860c5b4477851fb3790bc8068f808d3e/src/wallet/load.cpp#L79), [`RestoreWallet`](https://github.com/bitcoin/bitcoin/blob/c92fd638860c5b4477851fb3790bc8068f808d3e/src/wallet/wallet.cpp#L446), [`MigrateLegacyToDe
...
💬 Xekyo commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1227081979)
Thanks, done.
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1227081979)
Thanks, done.
💬 ryanofsky commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1587870978)
> > The only thing I think really should be addressed is the two BlockManager::Flush calls aborting without returning errors.
>
> I was about to open a separate PR from this branch here: https://github.com/TheCharlatan/bitcoin/tree/kernelReturnOnAbort
Great! I also opened #27862 early which overlaps with the branch a little. I think between the branch and PR all the cases where errors are not currently returned are covered.
(https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1587870978)
> > The only thing I think really should be addressed is the two BlockManager::Flush calls aborting without returning errors.
>
> I was about to open a separate PR from this branch here: https://github.com/TheCharlatan/bitcoin/tree/kernelReturnOnAbort
Great! I also opened #27862 early which overlaps with the branch a little. I think between the branch and PR all the cases where errors are not currently returned are covered.
💬 Xekyo commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1227089752)
I’ve reset it to (2, 5) after fuzzing ~100k execs and the problematic seed. I’m not sure (1, 5) would be an improvement in this particular fuzz seed, but happy to change it if people are convinced otherwise.
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1227089752)
I’ve reset it to (2, 5) after fuzzing ~100k execs and the problematic seed. I’m not sure (1, 5) would be an improvement in this particular fuzz seed, but happy to change it if people are convinced otherwise.
📝 achow101 opened a pull request: "wallet: Track no-longer-spendable TXOs separately"
(https://github.com/bitcoin/bitcoin/pull/27865)
In #27286, the wallet keeps track of all of its transaction outputs, even if they are already spent or are otherwise unspendable. This TXO set is iterated for balance checking and coin selection preparation, which can still be slow for wallets that have had a lot of activity. This PR aims to improve the performance of such wallets by moving UTXOs that are definitely no longer spendable to a different map in the wallet so that far fewer TXOs need to be iterated for the aforementioned functions.
...
(https://github.com/bitcoin/bitcoin/pull/27865)
In #27286, the wallet keeps track of all of its transaction outputs, even if they are already spent or are otherwise unspendable. This TXO set is iterated for balance checking and coin selection preparation, which can still be slow for wallets that have had a lot of activity. This PR aims to improve the performance of such wallets by moving UTXOs that are definitely no longer spendable to a different map in the wallet so that far fewer TXOs need to be iterated for the aforementioned functions.
...
📝 TheCharlatan opened a pull request: "validation: Return on abort"
(https://github.com/bitcoin/bitcoin/pull/27866)
This pull request is part of a larger effort towards improving the shutdown / abort / fatal error handling in validation code. It is a first step towards implementing proper fatal error return type enforcement similar as proposed by theuni in this pull request [comment](https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1563561502). For ease of review of these critical changes, a first step would be checking that `AbortNode` leads to early and error-conveying returns at its call site.
...
(https://github.com/bitcoin/bitcoin/pull/27866)
This pull request is part of a larger effort towards improving the shutdown / abort / fatal error handling in validation code. It is a first step towards implementing proper fatal error return type enforcement similar as proposed by theuni in this pull request [comment](https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1563561502). For ease of review of these critical changes, a first step would be checking that `AbortNode` leads to early and error-conveying returns at its call site.
...
👋 mzumsande's pull request is ready for review: "test: fix intermittent failure in p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/pull/27864)
(https://github.com/bitcoin/bitcoin/pull/27864)
💬 TheCharlatan commented on pull request "kernel: Remove shutdown globals from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1587883295)
I opened #27861 to introduce the interrupt class and implement the `fatalError` notification. Also opened https://github.com/bitcoin/bitcoin/pull/27866 to start tackling proper error return types when a function calls abort.
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1587883295)
I opened #27861 to introduce the interrupt class and implement the `fatalError` notification. Also opened https://github.com/bitcoin/bitcoin/pull/27866 to start tackling proper error return types when a function calls abort.
💬 Xekyo commented on pull request "doc: update comment in policy.cpp to refer to virtual bytes":
(https://github.com/bitcoin/bitcoin/pull/27726#discussion_r1227098886)
Yeah, this should be **vbytes** instead of bytes. The figure is also inaccurate, a P2WPKH input is [67.75 vbytes (for low r) or 68.0 vbytes for (high r)](https://bitcoin.stackexchange.com/a/100160/5406).
(https://github.com/bitcoin/bitcoin/pull/27726#discussion_r1227098886)
Yeah, this should be **vbytes** instead of bytes. The figure is also inaccurate, a P2WPKH input is [67.75 vbytes (for low r) or 68.0 vbytes for (high r)](https://bitcoin.stackexchange.com/a/100160/5406).
💬 achow101 commented on pull request "wallet: Migrate wallets that are not in a wallet dir":
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1227130800)
Done as suggested
(https://github.com/bitcoin/bitcoin/pull/26740#discussion_r1227130800)
Done as suggested
💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1587940978)
Addressed the feedback and rebased, which seems to have fixed the CI issues finally.
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1587940978)
Addressed the feedback and rebased, which seems to have fixed the CI issues finally.
🤔 ryanofsky reviewed a pull request: "validation: Return on abort"
(https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1475602374)
I'm not sure about all the changes in this PR because it seems like some validation code is intentionally written to ignore flush errors, and now they are returning early and skipping other work. It also seems reasonable that some failed flushes like periodic flushes might not need to be fatal errors.
But adding return values and nodiscard attributes could at least make it clear if errors are being ignored intentionally or not.
(https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1475602374)
I'm not sure about all the changes in this PR because it seems like some validation code is intentionally written to ignore flush errors, and now they are returning early and skipping other work. It also seems reasonable that some failed flushes like periodic flushes might not need to be fatal errors.
But adding return values and nodiscard attributes could at least make it clear if errors are being ignored intentionally or not.
💬 ryanofsky commented on pull request "validation: Return on abort":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1227107197)
In commit "blockstorage: Return on fatal flush errors" (1fe06a811bf8be941bef5336af97688f48885828)
This method is not marked nodiscard, and it seems like there are some cases where this error is ignored and not returned to the caller.
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1227107197)
In commit "blockstorage: Return on fatal flush errors" (1fe06a811bf8be941bef5336af97688f48885828)
This method is not marked nodiscard, and it seems like there are some cases where this error is ignored and not returned to the caller.
💬 ryanofsky commented on pull request "validation: Return on abort":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1227119951)
In commit "chainstate: Return on ValidatedSnapshotCleanup fatal failure" (360fe5a47f75514621ef62bc4c906f2112d3308f)
I think this should return FAILURE not INTERRUPTED so caller can know that an error happened, not just an early shutdown request.
But I'd actually suggest dropping this commit from the PR. #27862 should cover this case, and I opened it earlier because I think AbortNode calls in assumeutxo code and in flush code seem different. There should be less of a rationale for ignoring
...
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1227119951)
In commit "chainstate: Return on ValidatedSnapshotCleanup fatal failure" (360fe5a47f75514621ef62bc4c906f2112d3308f)
I think this should return FAILURE not INTERRUPTED so caller can know that an error happened, not just an early shutdown request.
But I'd actually suggest dropping this commit from the PR. #27862 should cover this case, and I opened it earlier because I think AbortNode calls in assumeutxo code and in flush code seem different. There should be less of a rationale for ignoring
...