💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2068074835)
> Embedding copies of shared code just makes things more fragile, not safer. It's far more likely there's a bugfix that we miss picking up on, than that there's malicious code inserted later.

In addition to what @laanwj said, it's also a much simpler re-implementation. The libevent function provided further options that we didn't care about and hardcoded, that's how we could simplify. And as was discussed in the last IRC meeting, there is also interest in removing libevent in general, at whi
...

💬 laanwj commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2068154367)
> So, to be clear, your opinion is for this part I should https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573347809 and introduce a change of behavior, right?

Yes. There's no use in emulating broken C string behavior.

⚠️ asctime opened an issue: "Crash on fail ~CCheckQueue() destructor?"
(https://github.com/bitcoin/bitcoin/issues/29930)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

This will crash the application when false:

```
~CCheckQueue()
{
assert(m_worker_threads.empty());
}
```

### Expected behaviour

Possibly do something more like this?
```

~CCheckQueue() {
try {
if (!m_worker_threads.empty()) {
// Attempt to stop worker threads if they haven't been stopped yet.
S
...

💬 luke-jr commented on pull request "ThreadSanitizer: Fix #29767":
(https://github.com/bitcoin/bitcoin/pull/29776#issuecomment-2068164464)
>It seems like the race condition between m_synced = true and committing has been around since 4368384 from #14121, and the race condition accessing the m_next_filter_pos variable existed then too.

Until #28955, `cs_main` was held across all this to prevent a race.

Not going to look at it right now, but I suspect this fix just creates a different race instead.

⚠️ asctime opened an issue: "ReadAnchor throws exception on second run"
(https://github.com/bitcoin/bitcoin/issues/29931)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

```
std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path)
{
std::vector<CAddress> anchors;
try {
DeserializeFileDB(anchors_db_path, CAddress::V2_DISK(anchors));
LogPrintf("Loaded %i addresses from %s\n", anchors.size(), fs::quoted(fs::PathToString(anchors_db_path.filename())));
} catch (const std::exception&) {
anchors.clear();

...

💬 1440000bytes commented on issue "Crash on fail ~CCheckQueue() destructor?":
(https://github.com/bitcoin/bitcoin/issues/29930#issuecomment-2068178196)
> Open settings in bitcoin-qt, adjust and save.

I am not able to reproduce this. What exactly did you adjust in settings before clicking on ok?

💬 luke-jr commented on pull request "ThreadSanitizer: Fix #29767":
(https://github.com/bitcoin/bitcoin/pull/29776#issuecomment-2068179144)
Looks like furszy already got this in #29867

💬 paplorinc commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573899433)
Thanks for adding me as a co-author, if you force push again, please change the email to `Lőrinc <pap.lorinc@gmail.com>`

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2068180640)
I have added the behavior change in a separate commit.

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573899901)
The behavior change is now re-introduced but explicitly documented as such in a separate commit.

💬 paplorinc commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573900516)
would it make sense to mention the `&` as well?

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573902309)
Are these commonly decoded as a space as well? I couldn't find evidence for this but I didn't spend much time on it.

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573902893)
> Thanks for adding me as a co-author, if you force push again, please change the email to `Lőrinc <pap.lorinc@gmail.com>`

fixed

💬 paplorinc commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573902969)
Not exactly: https://github.com/libevent/libevent/blob/release-2.1.12-stable/http.c#L3186-L3188

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573904156)
Hm, the code you are linking to is part of how libevent handles the `+`. Or did you want to refer to `?`? I think if it's not very common for `&` to be decoded as space then we don't need to mention it.

💬 paplorinc commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573905184)
? seems to be a control char, `If -1, when true we transform plus to space only after we've seen a ?. -1 is deprecated.`, no need to mention it, thanks for checking.

💬 asctime commented on issue "Crash on fail ~CCheckQueue() destructor?":
(https://github.com/bitcoin/bitcoin/issues/29930#issuecomment-2068196611)
Any setting, or just clicking ok (cancel is fine). It's probably something slightly different in MinGW pthread support.
so assert(m_worker_threads.empty()); causing a crash still analyzing.

So I understand assert will tell us what part of the code crashed, but it doesn't really tell us anything about the worker that wont stop, (thread id etc) so far it's only the settings box -> everything else seems to work after #29931 fingers crossed. gui syncing now.

💬 ismaelsadeeq commented on pull request "test: Refactor fee calculation to remove satoshi_round function":
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2068218318)
The C.I failure here is due to unused import @naiyoma
```
{'-zmqpubhashblock', '-zmqpubsequencehwm', '-zmqpubsequence', '-zmqpubrawblock', '-zmqpubhashtxhwm', '-zmqpubhashblockhwm', '-includeconf', '-zmqpubhashtx', '-zmqpubrawtx', '-testdatadir', '-zmqpubrawblockhwm', '-zmqpubrawtxhwm'}
test/functional/test_framework/util.py:8:1: F401 'decimal.ROUND_DOWN' imported but unused
^---- failure generated from lint-python.py

^---- ⚠️ Failure generated from lint-*.py scripts!
```

💬 luke-jr commented on pull request "Don't permit port in proxy IP option":
(https://github.com/bitcoin-core/gui/pull/813#issuecomment-2068230372)
@jarolrod Isn't that a bug independent from the validation one?

💬 fjahr commented on pull request "rpc: Optimize serialization disk space of dumptxoutset - Reloaded":
(https://github.com/bitcoin/bitcoin/pull/29612#discussion_r1573943040)
done