🤔 furszy reviewed a pull request: "wallet: simplify and batch zap wallet txes process"
(https://github.com/bitcoin/bitcoin/pull/28987#pullrequestreview-1872874268)
Updated per feedback. Thanks Marko and achow.
(https://github.com/bitcoin/bitcoin/pull/28987#pullrequestreview-1872874268)
Updated per feedback. Thanks Marko and achow.
💬 furszy commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1484621811)
> I'm not sure if `NONCRITICAL_ERROR` is really the right return code. It seems like something has gone critically wrong if `TxnCommit` were to fail.
The returned error code doesn't really matter. Anything different from `LOAD_OK` is an error for all the function's callers. And they behave exactly the same for all codes.
Actually, the `DBErrors` return doesn't make sense anymore after this PR changes. The function is no longer traversing the entire db nor checking for extra, unneeded, stuff
...
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1484621811)
> I'm not sure if `NONCRITICAL_ERROR` is really the right return code. It seems like something has gone critically wrong if `TxnCommit` were to fail.
The returned error code doesn't really matter. Anything different from `LOAD_OK` is an error for all the function's callers. And they behave exactly the same for all codes.
Actually, the `DBErrors` return doesn't make sense anymore after this PR changes. The function is no longer traversing the entire db nor checking for extra, unneeded, stuff
...
💬 furszy commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1484606675)
> This should explicitly call TxnAbort before returning as otherwise we will get extraenous logs about the tx being aborted before batch is destroyed.
sure. Fixed.
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1484606675)
> This should explicitly call TxnAbort before returning as otherwise we will get extraenous logs about the tx being aborted before batch is destroyed.
sure. Fixed.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484624000)
Done
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484624000)
Done
🤔 TheCharlatan reviewed a pull request: "RFC: build: remove confusing and inconsistent disable-asm option"
(https://github.com/bitcoin/bitcoin/pull/29407#pullrequestreview-1872906747)
Do the mentiones from the `doc/fuzzing.md` to `--disable-asm` still need to be removed? Seems like `--disable-asm` is not recognized by libsecp, it supports a package-specific asm option.
(https://github.com/bitcoin/bitcoin/pull/29407#pullrequestreview-1872906747)
Do the mentiones from the `doc/fuzzing.md` to `--disable-asm` still need to be removed? Seems like `--disable-asm` is not recognized by libsecp, it supports a package-specific asm option.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484625047)
Yes, I've added a comment for that.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484625047)
Yes, I've added a comment for that.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484626857)
I have added a lot more comments to the test which should clarify this.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484626857)
I have added a lot more comments to the test which should clarify this.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484632206)
I have added a comment before all of the tests in this file explaining how these tests create conflicts.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484632206)
I have added a comment before all of the tests in this file explaining how these tests create conflicts.
💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484632803)
Done
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484632803)
Done
✅ achow101 closed an issue: "Enable sanctions enforcement in Spam Filter Code"
(https://github.com/bitcoin/bitcoin/issues/29416)
(https://github.com/bitcoin/bitcoin/issues/29416)
💬 pablomartin4btc commented on pull request "doc: Update translation process guide":
(https://github.com/bitcoin/bitcoin/pull/29414#discussion_r1484665290)
I'd leave the `~/.transifexrc` in case the user creates an account in Transifex directly, instead of using another platform account like GitHub.
(https://github.com/bitcoin/bitcoin/pull/29414#discussion_r1484665290)
I'd leave the `~/.transifexrc` in case the user creates an account in Transifex directly, instead of using another platform account like GitHub.
💬 achow101 commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#issuecomment-1936426651)
ACK 9a3c5c8697659e34d0476103af942a4615818f4e
The new code is much simpler.
(https://github.com/bitcoin/bitcoin/pull/28987#issuecomment-1936426651)
ACK 9a3c5c8697659e34d0476103af942a4615818f4e
The new code is much simpler.
💬 achow101 commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#issuecomment-1936428779)
ACK 8330145ba88b39ddaa9e0bbfc2d1c198da652249
(https://github.com/bitcoin/bitcoin/pull/29403#issuecomment-1936428779)
ACK 8330145ba88b39ddaa9e0bbfc2d1c198da652249
💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1936432223)
ACK 13161ecf032b7a850686e5942c12222c8f3d0d52
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1936432223)
ACK 13161ecf032b7a850686e5942c12222c8f3d0d52
💬 sr-gi commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1936440669)
tACK [c10f528](https://github.com/bitcoin/bitcoin/pull/27114/commits/c10f528350152ca9248e8c167b67993fc7321ca3)
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1936440669)
tACK [c10f528](https://github.com/bitcoin/bitcoin/pull/27114/commits/c10f528350152ca9248e8c167b67993fc7321ca3)
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484694946)
Ah, I see. That's unfortunate as it means this optimization isn't available before some solution was found, but probably doesn't matter much.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484694946)
Ah, I see. That's unfortunate as it means this optimization isn't available before some solution was found, but probably doesn't matter much.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484695646)
Oh, I missed that this was just changing the expected result, not the input. Sounds good.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484695646)
Oh, I missed that this was just changing the expected result, not the input. Sounds good.
💬 ManuelSchneid3r commented on issue "Weird focus rect displayed on inital sync":
(https://github.com/bitcoin-core/gui/issues/783#issuecomment-1936463536)
I just started it. yes Actually I deactivated the app to let the focus disappear.
(https://github.com/bitcoin-core/gui/issues/783#issuecomment-1936463536)
I just started it. yes Actually I deactivated the app to let the focus disappear.
🤔 mzumsande reviewed a pull request: "assumeutxo: Get rid of faked nTx and nChainTx values"
(https://github.com/bitcoin/bitcoin/pull/29370#pullrequestreview-1871315375)
Concept ACK
This looks like a nice simplification.
(https://github.com/bitcoin/bitcoin/pull/29370#pullrequestreview-1871315375)
Concept ACK
This looks like a nice simplification.
💬 mzumsande commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1484708844)
The next paragraph refers to this deleted comment ("using
the nStatus flag mentioned above") and should be updated too.
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1484708844)
The next paragraph refers to this deleted comment ("using
the nStatus flag mentioned above") and should be updated too.