💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1482110161)
Sure. `__func__ ` removed.

💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1482110377)
Sure. `__func__ ` removed.

🤔 furszy reviewed a pull request: "wallet: batch and simplify addressbook migration process"
(https://github.com/bitcoin/bitcoin/pull/26836#pullrequestreview-1868826371)
Updated per feedback. Thanks achow!. [Small diff](https://github.com/bitcoin/bitcoin/compare/1f177ff9a6ab229bd6486941e46daa92ab22b622..86960cdb7f75eaa2ae150914c54240d1d5ef96d1).

Two changes:
1) Removed `__func__ ` usages in the logging messages.
2) Have divided the purpose and name erasing lines per [request](https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1481997602).

💬 ziljah commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1932967456)
👎

🤔 sr-gi reviewed a pull request: "rpc: addpeeraddress tried return error on failure"
(https://github.com/bitcoin/bitcoin/pull/28998#pullrequestreview-1868852542)
Approach ACK

It is a pity that `"failed-adding-to-tried"` cannot be tested on demand, however, this at least gets rid of the intermittent failure when a collision is found in `tried`.

I think the tests can be made more exhaustive though, check comment inline.

💬 sr-gi commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1482129839)
This is a non-exhaustive case now. There are two potential error outcomes that can be triggered on demand, from which only one is being tested:

- a) The address is already in `new` and we are trying to add it again (to any table)
- b) The address is already in `tried` and we are trying to add it again (to any table)

In this case, only `b)` is being tested: we do so both with and without the "try to add to tried" flag, but the reality is that the reason why the function is failing is the e
...

💬 achow101 commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1933039793)
ACK 86960cdb7f75eaa2ae150914c54240d1d5ef96d1

🤔 furszy reviewed a pull request: "wallet: batch erase procedures and improve 'EraseRecords' performance"
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1868847059)
Updated per feedback. Thanks achow!

💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1482179514)
> Instead of `assert`ing, I would prefer if this did `if (!Assume(activeTxn)) return false` so we didn't cause nodes to crash if a transaction were missing for whatever reason. `Assume` should still catch this for developers.

That would be risky in terms of consistency. `BerkeleyBatch::ErasePrefix` traverses the entire db, reading entries one by one, erasing the ones that match the key prefix. If we don't execute this function within a txn context, it's possible that certain records are remov
...

💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1482126505)
> I'd prefer to have these occur separately so that a distinct error can be logged for each, that way we can discover which operation failed.

Sure. Also decoupled it in #26836.

💬 furszy commented on pull request "test: handle wallet_reorgrestore.py failed":
(https://github.com/bitcoin/bitcoin/pull/29395#discussion_r1482188486)
These changes aren't doing anything. The sync call is being done after obtaining the data from the node.

💬 achow101 commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1482191573)
Sure, that's why I suggest using `Assume`, and `return false` if there is no `activeTxn`. The callers of this should already be checking the return value to know whether the records were actually erased from the database.

💬 epiccurious commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1933069979)
utACK 6312513b5074f160971ffec0b093b58b9d3cdaae. Planning to test as well.

👍 johnny9 approved a pull request: "release: Update translations for v27.0 soft translation string freeze"
(https://github.com/bitcoin/bitcoin/pull/29397#pullrequestreview-1869011331)
ACK 71927b24e5aceecd8a07cdaeb916898d45486bea

Reproduced by running update-translations.py and then `./configure && make -C src translate`. No diff when comparing my branch and this one.

https://github.com/hebasto/bitcoin/compare/230207-translation...johnny9:bitcoin:translation-check-27?expand=1

💬 ariard commented on issue "Update security.md with all PGP fingerprints":
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1933238232)
As usual with any question posed in the OP, wait or ask first if the opener is satisfied (modulo basic trolling / gpt-spam ofc).

I’m asking a) for the full sec-list to be public b) for each member to have a PGP fingerprint available and c) a dedicated sec mail address for each endpoint instead or in redundancy of using an alias. I understand and I know the risk of targeted attacks on members, I think it has to be weighted with better accountability.

PGP every communications of sensitive in
...

💬 ariard commented on issue "Update security.md with all PGP fingerprints (version 2)":
(https://github.com/bitcoin/bitcoin/issues/29383#issuecomment-1933242093)
> Don't open duplicate issues. If you think an issue should be reopened, then say so in that issue.

Always making my own assumptions on online behavior of anyone in this space. Thanks.
Saying this, with respect I have for GH maintenance work.

✅ ryanofsky closed an issue: "ci: failure in `wallet_basic.py --descriptors`"
(https://github.com/bitcoin/bitcoin/issues/29110)

🚀 ryanofsky merged a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112)

💬 daniel-btc-nym commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1933300387)
The bug here isn't actually that bad:

- It uses strncmp(..., 4) and strncmp(..., 5) respectively: so, in worst case it will read 3 or 5 bytes after end.
- Similar for the number parsing.

In any case, I have a change that fixes both (adding guards for reading past end, and making the code pretty / easy to read again). I have unit tests for the keywords (true, false, null) but want to add some for the numbers.

Haven't made any progress on MSAN. But if we would poison data around begin
...

💬 hebasto commented on pull request "build: disable external-signer for Windows":
(https://github.com/bitcoin/bitcoin/pull/28967#issuecomment-1933377466)
As it seems that https://github.com/bitcoin/bitcoin/pull/28981 won't make it into v27.0, I suggest reflecting the disabling of external signer support for Windows in the release notes. This will allow users who rely on it to skip the next release update.